Type changes for modules over non commutative rings

[HechtiDerLachs]

Building on the work by @Lax202 in #3900, I deliberately extended the signatures to also allow for rather general non-commutative rings. I tested this code locally in another branch with a generic type for exterior algebras and the changes you see here were those necessary to make everything run.

This is a suggestion only. If people feel uncomfortable with the generality here, please feel free to close the PR.

Note that reasonable testing is not yet possible, since the latest Hecke version is needed for that.

[HechtiDerLachs]

I added some more things which I found to be necessary to make modules over non-commutative rings work.

Among other things I did some profiling with my code and also added some fixes to save resources in general.

[codecov]

Codecov Report

Attention: Patch coverage is 68.88889% with 14 lines in your changes missing coverage. Please review.

Project coverage is 84.64%. Comparing base (97bc0a4) to head (046c51f).
Report is 2 commits behind head on master.

Files with missing lines	Patch %	Lines
src/Modules/UngradedModules/FreeModElem.jl	36.84%	12 Missing ⚠️
src/Modules/ModulesGraded.jl	90.00%	1 Missing ⚠️
src/Modules/UngradedModules/SubQuoHom.jl	75.00%	1 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #3988      +/-   ##
==========================================
- Coverage   84.65%   84.64%   -0.01%     
==========================================
  Files         614      614              
  Lines       83344    83349       +5     
==========================================
- Hits        70554    70550       -4     
- Misses      12790    12799       +9     

Files with missing lines	Coverage Δ
...try/Schemes/ProjectiveSchemes/Morphisms/Methods.jl	61.53% <100.00%> (ø)
src/Modules/ModuleTypes.jl	78.90% <100.00%> (ø)
src/Modules/UngradedModules/FreeMod.jl	84.31% <100.00%> (ø)
src/Modules/UngradedModules/SubquoModule.jl	75.53% <100.00%> (ø)
src/Modules/UngradedModules/SubquoModuleElem.jl	84.21% <100.00%> (ø)
src/Modules/ModulesGraded.jl	73.72% <90.00%> (ø)
src/Modules/UngradedModules/SubQuoHom.jl	77.60% <75.00%> (ø)
src/Modules/UngradedModules/FreeModElem.jl	85.71% <36.84%> (-10.29%)	⬇️

... and 1 file with indirect coverage changes

[joschmitt]

Would it be possible to put the fixes and performance optimizations in a separate pull request? I assume we definitely want to have 76c3599 independent of whether the decision is to have modules over non-commutative rings in this generality or not.

[HechtiDerLachs]

Yes, I can do that. But I wanted to first get a full picture of how this should be split up or not. If everything was approved eventually, then I could save the extra time on the manual splitting and testing, right? If it's urgent, though, I can separate certain things. Let me know if this is the case.

Edit: Also, I think, it would make sense to include the tests written by @Lax202 . They just don't run at this point, because a later Hecke version is required (one which accepts sparse rows over non-commutative rings).

Edit: In the end I think, you're right. We will probably want the changes in different patches either way.

[joschmitt]

Yes, I can do that. But I wanted to first get a full picture of how this should be split up or not. If everything was approved eventually, then I could save the extra time on the manual splitting and testing, right? If it's urgent, though, I can separate certain things. Let me know if this is the case.

Edit: Also, I think, it would make sense to include the tests written by @Lax202 . They just don't run at this point, because a later Hecke version is required (one which accepts sparse rows over non-commutative rings).

I just think it would make sense to keep fixes separate from new features if possible. None of this is urgent (for me), but I assume that the whole pull request has to wait for some people to return from holidays, a new Hecke release and in general people to agree on something to be merged. The fix for the radical or the optimization with making degrees_of_generators an attribute for example should be non-controversial and could be merged now.

[HechtiDerLachs]

I updated this PR to only comprise the changes necessary to allow for non-commutative rings. Everything else should already be in #3995 .

Still I think we should carry over the tests due to @Lax202 from #3900. But this requires the new Hecke which we don't have yet.

ping @jankoboehm

[joschmitt]

Still I think we should carry over the tests due to @Lax202 from #3900. But this requires the new Hecke which we don't have yet.

There was a Hecke release yesterday.

[HechtiDerLachs]

Looks like the Hecke release is breaking the doctests?

[joschmitt]

Looks like the Hecke release is breaking the doctests?

@thofma ?

[thofma]

Was not caught in the downstream tests unfortunately, but should be fixed by #3999.

[HechtiDerLachs]

Tests pass.

We need to decide on the following:

1. Do we want to allow modules over non-commutative rings in general? In For Modules over PBWAlgRing and PBWAlgQuo #3900 the signatures were only widened to handle PBWAlgElems, but not NCRingElems. Personally, I do not see a good reason to make such restrictions, but maybe people had different ideas about this?
2. How do we handle right-multiplication? At the moment every ModuleFP over a non-commutative ring is a priori considered to be a left-module. For subquos, I'm afraid, we have to stick to such a convention/restriction, eventually. But for free modules and SubModuleOfFreeModule we could in principle also allow for right-multiplication. At the moment some methods for this exist, but they will throw an error. I think this is a good compromise for the time being so that no wrong code is introduced, but yet the whole functionality can be extended if we decide to do so.
3. @Lax202 put some good work into making SRows accept non-commutative rings. Sparse matrices, however, do not work over such rings, yet. They are used for modules in various places, even though not at the level of core functionality. But we should support SMat over non-commutative rings.

For a full fledged support of left-, right-, and bi-modules we should then eventually discuss whether it might be worth introducing new types. Alternatively, one could probably also go via attributes of the existing modules, but as you can guess I would not be a fan of such an approach. I think some stuff is already done for ideals and we could peak at the structure there to learn? Either way: I think this PR can be merged without going into that discussion for the moment: All modules are naturally treated as left-modules for the time being and that can be extended at a later date to whatever we decide to do.

[thofma]

If I remember correctly, one of Oscars design principle says that we use right actions, so the modules should be right modules by default. It is also the convention that we use for the modules over noncommutative rings that we have already, e.g., modules over finite-dimensional algebras.

Maybe it is a also a good question to ask what the goal of these modules is? Catching all NCRingElem makes little sense, since the concepts that are trying to be used here (normal forms? standard bases? subquo?) are applicable only to a very small subset of modules/rings. So why not stick PWB algebras, where this has a chance of working?

[wdecker]

If I remember correctly, one of Oscars design principle says that we use right actions, so the modules should be right modules by default. It is also the convention that we use for the modules over noncommutative rings that we have already, e.g., modules over finite-dimensional algebras.

In the docu we write: When implementing functionality for PBW-algebras, taking the opposite algebra into account often allows one to focus on left ideals, left modules, and left Gröbner bases. So under the hood, when it comes to computations, we follow the principle of Singular. Nevertheless, for the applications, we need all three types of directions: left, right, and twosided (some operations may even change the direction). Accordingly, we have mutable struct PBWAlgIdeal{D, T, S}, where D stands for the direction. For me it does not make sense to have one of these directions as the default. I think you cannot compare the situation here with the situations where the above mentioned design principle applies.

[thofma]

It is fine with me if this is how the PBW algebras and their modules should be implemented. But what is implemented here is

Oscar.jl/src/Modules/UngradedModules/FreeModElem.jl

Lines 264 to 270 in 9b269b8

	function *(b::AbstractFreeModElem{T}, a::T) where {T <: NCRingElem}
	error("all modules are left-modules by default")
	end
	function *(b::AbstractFreeModElem{T}, a::Any) where {T <: RingElem}
	error("scalar multiplication from the right is not defined; use left-multiplication instead")
	end

which is, I think, both not adhering to the Oscar standard and against the idea to not have a default.

[HechtiDerLachs]

one of Oscars design principle says that we use right actions, so the modules should be right modules by default.

I was afraid this was the case. However, this would be a whole different load of work! All of the code for ModuleFPs has originally been developed for commutative rings and the implementation uses left-multiplication. If we were to make the ModuleFPs right modules by default, we would have to go through the entire code base and exchange a*v by v*a for scalars a and ModuleFPElems/SRows/... v. We can do that, but that would be an expensive task and not covered in this small PR.

Catching all NCRingElem makes little sense

So does catching all RingElems for the existing implementations. Yet, the signatures said Ring and RingElem and not MPolyRing and MPolyRingElem. For the generic types I would therefore be in favor of widening the signature to NCRingElem, regardless of what implementations we really do offer in the end.

For example, I have implemented a custom type ExteriorAlgebra{T} for exterior algebras over arbitrary rings. This is not a PBWAlgebra, because the ground ring is not necessarily a field and I wanted to be able to write generic code which does not bind to the singular backend directly. I am using the ModuleFPs over ExteriorAlgebra{T}s and it works just fine. But that would not be possible, if the signature was restricted to PBWAlgElems. Plus: If we made that restriction, then we would also disallow external users to use the Oscar modules with their custom non-commutative rings.

[HechtiDerLachs]

which is, I think, both not adhering to the Oscar standard and against the idea to not have a default.

Probably true. I would like to hear an executive decision about this. Personally, I do not really care, but this should be decided somehow. These lines are just a minimal fix to make things half-way coherent for the moment. And given that, as I explained above, the current implementation of the generic module code is unable to support right-modules, this is the only feasible solution for the moment.

[wdecker]

both not adhering to the Oscar standard and against the idea to not have a default.

@thofma Oh, I see now. I perfectly agree with what you write.

[thofma]

one of Oscars design principle says that we use right actions, so the modules should be right modules by default.

I was afraid this was the case. However, this would be a whole different load of work! All of the code for ModuleFPs has originally been developed for commutative rings and the implementation uses left-multiplication. If we were to make the ModuleFPs right modules by default, we would have to go through the entire code base and exchange a*v by v*a for scalars a and ModuleFPElems/SRows/... v. We can do that, but that would be an expensive task and not covered in this small PR.

First of all, I am sympathetic to small changes that enable some nice applications. But the fundamental design decision on how modules over noncommutative rings should be handled is probably beyond a small PR:

• The concept of left/right/bi and how it is handled is not thought through in my opinion.
• This is not something experimental and once it is in, it will never be done properly.
• It should take into account what is there already in terms of modules over noncommutative rings and should not go exactly the opposite direction.

So I don't thinks this is a good argument for going against the design principle and the rest of Oscar.

Changing the signature to only allow PBWAlgebra would be a hack to get something working, without us committing to some design. Although given the answer by Wolfram, I am also not sure if this is what is required/wished for in case of PBWAlgebras.

[HechtiDerLachs]

But the fundamental design decision on how modules over noncommutative rings should be handled is probably beyond a small PR

Given the above discussion, you are probably right. I understand your concerns. Even though this is rather sad, because I would have liked to have these changes supported in Oscar with a view towards cohomology computations via exterior algebras. But if I have to wait for a coherent framework of modules over non-commutative rings to first be developed, then that is what it is. In that case we can close the PR and just continue with #3900.

[jankoboehm]

There is a chance that we can follow the situation for ideals. Then what we have in this PR would exactly be what is needed, there is just some logic missing on top: We reduce the right case to the left, and the two-sided to the right. What would be needed is that the underlying algorithmic oracles can support that. Groebner bases should.

[wdecker]

two-sided to the right.

For PBW algebras we compute a LEFT GB first and enlarge is successively until it is a right GB, too.

[HechtiDerLachs]

I want to apologize to anyone involved here. Today I was informed that similar discussions had already taken place elsewhere, also offline. I didn't mean to overthrow or question the considerations and conclusions that were eventually reached at other occasions. I suggest to continue any discussion about these matters in person when we meet again.

[HechtiDerLachs]

A first write-up of today's discussion.

In addition, we will insert a type union AdmissibleModuleFPRingElem = Union{<:RingElem, <:PBWAlgElem, <:PBWAlgQuoElem, ...} and use that in the restriction for the type parameters of the module types and functions.

In the long run we plan to support right-modules similar to the case of ideals: For an NCRing A there is an "opposite algebra" A_op and a bijective morphism phi : A -> A_op. This should extend generically to modules phi : M -> M_op. For a right-module M we can then define multiplication for elements a in A and v in M via
v*a = inv(phi)(phi(a)*phi(v)). The opposite algebra and -modules are, of course, cached.

[thofma]

I had a short discussion with @wdecker and we concluded that there should be left_free_module/right_free_module, but no free_module for noncommutative rings.

[HechtiDerLachs]

What happened to this PR? I tried to push some more fixes, but it rejected me. When I pulled, I got all that stuff from other people. Was this a mistake?

[HechtiDerLachs]

Tests pass. From my side this is good to go.

[thofma]

Does this here only implement FreeMod but not free_module?

[HechtiDerLachs]

Yes, I think, I took out the `free_module` implementation from Lakshmi already before for the obvious reasons. But that reminds me that I didn't yet add the other constructors.

[thofma]

OK. Not sure what you plan to add exactly, but with @wdecker we agreed to not have free_module but only left_free_module/right_free_module in the general (noncommutative) case. We can also talk about this on Monday.

[HechtiDerLachs]

Yes, let's finalize this on Monday together then.