sparse_row and SRow accept NCRing instead of Ring

[Lax202]

This is to be able to create Modules in Oscar over non-commutative rings, as discussed with @HechtiDerLachs and @fieker

[lgoettgens]

There is some of functionality for SRows that expects the coefficient ring to be commutative. These need to be adapted or restricted to elements of subtypes of Ring.
Some that I found in a quick look:
https://github.com/Lax202/Hecke.jl/blob/41678ac4bc55f85a04da7074cdf93f7849ef6406/src/Sparse/Row.jl#L397-L399
Is scale_row!, div, divexact based on left- or right-multiplication? This should be decided and added to the docstrings. Furthermore, there should be functions for the other case as well.

[fingolfin]

In particular things like scale_row! may need two versions, for left and right action...

A crucial aspect of modules over non-commutative rings is that one has to be careful about which actions are left and which are right otherwise things don't work out in the end...

(@mohamed-barakat is an expert on this, perhaps we can consult with him, too ;-) but in principle I think I know the rules, too)

[Lax202]

I have added left and right actions in scale_row, add_scaled_row and divexact (not div). There are tests for everything except divexact since I am unable to find a non-commutative ring for which divexact_left is implemented.
The tests use an exterior algebra. This ring does not exist in Hecke. If someone could suggest an Hecke NCRing that can be used for tests, that would be great.

[thofma]

R = MatrixAlgebra(QQ, 2) and a = R([1 2; 3 4]) if you want an element. Or rand(R, -10:10) for something random.

[fingolfin]

Someone needs to allow the tests to run :-)

[thofma]

As said, the tests will fail, so no need to approve anything. I have to reapprove for the next commit anyway.

[mohamed-barakat]

A crucial aspect of modules over non-commutative rings is that one has to be careful about which actions are left and which are right otherwise things don't work out in the end...

(@mohamed-barakat is an expert on this, perhaps we can consult with him, too ;-) but in principle I think I know the rules, too)

I am happy to help, of course :) If you have specific questions please let me know.

[joschmitt]

@Lax202 I just pushed a commit to resolve the conflicts.

[fingolfin]

@Lax202 it would be good if you implemented the suggestions from the feedback here or figure out why it can't be done. Feel free to talk to me about details.

[fingolfin]

The main point of this is that it in fact modifies a in-place (otherwise you'd just write b*a or a*b). The docstring should make this clear.

[fingolfin]

Suggested change

	divexact(A::SRow, b::RingElem; check::Bool = true) -> SRow
	divexact(A::SRow, b::RingElem; check::Bool = true) -> SRow

[fingolfin]

Could someone (@thofma @joschmitt) please allow the CI tests to run?

[fingolfin]

Instead of duplicating all this code, you could also use Val to do something like this:

Suggested change

	function add_right_scaled_row!(a::SRow{T}, b::SRow{T}, c::T) where T
	function add_scaled_row!(a::SRow{T}, b::SRow{T}, c::T, ::Val{left} = Val(true)) where {T, left}

and then in the critical part you can do

  if left
    t = mul!(t, c, a.values[i])
  else
    t = mul!(t, a.values[i], c)
  end

But this can also wait for a future cleanup.
Then finally you can do

add_left_scaled_row!(a::SRow{T}, b::SRow{T}, c::T) where T = add_scaled_row!(a,b,c,Val(true))
add_right_scaled_row!(a::SRow{T}, b::SRow{T}, c::T) where T = add_scaled_row!(a,b,c,Val(false))

[fingolfin]

Again someone needs to allow the CI tests to run, please

[fingolfin]

@Lax202 the documentation tests still have some failures you need to address (we discussed these already):

ExpandTemplates: expanding markdown templates.
Error: no docs found for 'sparse_row(::ZZRing, ::Vector{Tuple{Int, ZZRingElem}})' in `@docs` block in src/manual/misc/sparse.md:30-34
```@docs
sparse_row(::ZZRing, ::Vector{Tuple{Int, ZZRingElem}})
sparse_row(::ZZRing, ::Vector{Tuple{Int, Int}})
sparse_row(::ZZRing, ::Vector{Int}, ::Vector{ZZRingElem})
```

[codecov]

Codecov Report

Attention: Patch coverage is 35.71429% with 54 lines in your changes missing coverage. Please review.

Project coverage is 75.62%. Comparing base (32b8b74) to head (2e90d2c).
Report is 22 commits behind head on master.

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #1322      +/-   ##
==========================================
+ Coverage   75.55%   75.62%   +0.06%     
==========================================
  Files         355      357       +2     
  Lines      113364   113521     +157     
==========================================
+ Hits        85655    85852     +197     
+ Misses      27709    27669      -40     

Files	Coverage Δ
src/HeckeTypes.jl	84.36% <62.50%> (ø)
src/Sparse/Row.jl	73.39% <32.89%> (-10.08%)	⬇️

... and 58 files with indirect coverage changes

[fingolfin]

@thofma @joschmitt @fieker can someone please approve the CI runs? @Lax202 just pushed an update which hopefully will address the documenter issue