Rewrite MatRing to wrap a native matrix

[fingolfin]

This is code trying to address issue #1955 which I wrote last December but never finished.

@JohnAAbbott I am putting it here because it might be useful for you, e.g. you could use it as a basis and complete it (feel free to work on this PR, or copy it to a new one). Or perhaps you prefer to start from scratch, but then it might still be helpful to check what I've done here.

This is incomplete in that I am sure tests will fail and things are missing. A lot of code which is currently defined for MatrixElem may have to be changed (but it may also be possible to delay that for the time being -- e.g. changing det to delegate to det of the wrapped matrix is strictly speaking an optimization?)

[JohnAAbbott]

Comment/Question about matrix multiplication (and probably other binary operators): the current proposal is for a function *(M1::MatrixElem, M2::MatrixElem) to exist. Since MatrixElem is a union of 2 types this "single" function must cover 4 cases, namely (MatElem, MatElem), (MatElem, MatRingElem), (MatRingElem, MatElem) and (MatRingElem, MatRingElem).
Will the Julia compiler produce 4 distinct compiled functions? The two "homogeneous" cases are clearly wanted; do we also want the two "heterogeneous" cases? Does it make sense to multiply a MatElem by a MatRingElem? Bear in mind that there is a trivial "conversion" from a MatRingElem to a MatElem -- it is just a member access (in the proposed redesign).

[thofma]

I think the mixed-type cases must be removed. So if we still need this, it should be *(::T, ::T) where {T <: ...}.

[JohnAAbbott]

I think the mixed-type cases must be removed. So if we still need this, it should be *(::T, ::T) where {T <: ...}.

Sorry, it was my misreading/misremembering of the code. Anyway, the only cases of interest are the two "homogeneous" cases. Thanks for the feedback! I'll deal with it tomorrow.
Would the following design be OK?

function *(M1::MatRingElem{T}, M2::MatRingElem{T})  where ...
  return M1.data * M2.data
end

Then the "real" function takes two MatElem values, and does all checking & computation.
It'd be nice to have a proper accessor function instead of using an explicit field accessor; any suggestions for a name?

[thofma]

Yes, sounds good. I think data seems to be a common name for this (there are already uses of this).

[fingolfin]

@JohnAAbbott ... while this was restricted to the "both arguments have the same type.

But yeah it really should be

Suggested change

	function *(x::T, y::T) where {T <: MatrixElem}
	function *(x::T, y::T) where {T <: MatElem}

and then there should be MatRingElem methods for *, +, - etc. which delegate to the the underlying "plain" matrix.

I.e., also the existing changes in this PR in lines 826 and 837 of this file need to be furhter modified.

[JohnAAbbott]

Strange: I thought I had already written that the suggested change triggers a StackOverflowError: namely

Views: Error During Test at /home/zez25zuh/OSCAR/TMP/AbstractAlgebra.jl/ext/TestExt/Rings-conformance-tests.jl:706
  Test threw exception
  Expression: N1 * N2 == matrix(R, 2, 2, BigInt[14, 20, 20, 29])
  StackOverflowError:
  Stacktrace:
   [1] Array
     @ ./boot.jl:479 [inlined]
   [2] MatSpaceElem
     @ ~/OSCAR/TMP/AbstractAlgebra.jl/src/generic/GenericTypes.jl:1114 [inlined]
   [3] _change_base_ring
     @ ~/OSCAR/TMP/AbstractAlgebra.jl/src/Matrix.jl:6546 [inlined]
   [4] change_base_ring(R::AbstractAlgebra.Integers{BigInt}, M::AbstractAlgebra.Generic.MatSpaceElem{BigInt})
     @ AbstractAlgebra ~/OSCAR/TMP/AbstractAlgebra.jl/src/Matrix.jl:6555
   [5] promote
     @ ~/OSCAR/TMP/AbstractAlgebra.jl/src/Matrix.jl:1170 [inlined]
   [6] *(x::AbstractAlgebra.Generic.MatSpaceView{BigInt, Tuple{UnitRange{Int64}, Base.Slice{Base.OneTo{Int64}}}, false}, y::AbstractAlgebra.Generic.MatSpaceElem{BigInt}) (repeats 79980 times)
     @ AbstractAlgebra ~/OSCAR/TMP/AbstractAlgebra.jl/src/Matrix.jl:1178

All tests pass with the code in its current state: 2025-12-10 16:38 CET

[JohnAAbbott]

I'm making progress, and thanks to @fieker have fixed one "type instability" problem -- the code is/was type-stable but Julia was not able to recognise this, so needed an explicit helping hand.

[JohnAAbbott]

What matrix operations do we want to perform on MatRingElem values?
A specific "problematic" instance is hnf_kb_with_transform: in the test file generic/MatRing-test.jl near line 1393 there is

   H, U = @inferred AbstractAlgebra.hnf_kb_with_transform(A)

which fails because

  Got exception outside of a @test
  return type Tuple{AbstractAlgebra.Generic.MatRingElem{AbstractAlgebra.Generic.Poly{Rational{BigInt}}}, AbstractAlgebra.Generic.MatRingElem{AbstractAlgebra.Generic.Poly{Rational{BigInt}}}} does not match inferred return type Tuple{AbstractAlgebra.Generic.MatRingElem{AbstractAlgebra.Generic.Poly{Rational{BigInt}}}, AbstractAlgebra.Generic.MatRingElem}

So the types almost match...
BUT do we really want to be able to apply hnf_kb_with_transform to a MatRingElem?

[JohnAAbbott]

Here are some functions for MatrixElem which I think should be defined
only for MatElem and not for MatRingElem. There are also several functions
defined for Union{Matrix, MatrixElem} -- is this really desired?

function check_square(A::MatrixElem{T}) where T <: NCRingElement
function (s::MatSpace{T})(a::MatrixElem{T}) where {T <: NCRingElement}

is_zero_entry(M::Union{Matrix,MatrixElem}, i::Int, j::Int)
is_positive_entry(M::Union{Matrix,MatrixElem}, i::Int, j::Int)
is_negative_entry(M::Union{Matrix,MatrixElem}, i::Int, j::Int)

function is_zero_row(M::Union{Matrix,MatrixElem}, i::Int)
function is_zero_column(M::Union{Matrix,MatrixElem}, j::Int)


Base.firstindex(M::MatrixElem{T}, i::Int) where T <: NCRingElement = 1
function Base.lastindex(M::MatrixElem{T}, i::Int) where T <: NCRingElement

Do we want an "Array interface" for MatRingElem?
i.e. Base.ndims, Base.eachindex, Base.getindex, Base.setindex!, Base.iterate,...

These function would allow a mixture of MatElem and MatRingElem as arguments -- desired?

function add!(c::MatrixElem{T}, a::MatrixElem{T}, b::MatrixElem{T}) where T <: NCRingElement
function mul!(c::MatrixElem{T}, a::MatrixElem{T}, b::MatrixElem{T}) where T <: NCRingElement
function sub!(c::MatrixElem{T}, a::MatrixElem{T}, b::MatrixElem{T}) where T <: NCRingElement

There are very many more in src/Matrix.jl (file is 7200 lines long)

The following will give an error if the matrix has 0 rows or 0 columns; intentional?
canonical_unit(a::MatrixElem{T}) where T <: NCRingElement = canonical_unit(a[1, 1])

[JohnAAbbott]

It is a significant hindrance that the tests call several of the above "questionable" functions. I prefer not to "push" while I know that several tests do not pass... The current blocking case is Vector times MatRingElem (and vice versa). I suspect that the tests are more-or-less a copy of the tests for matrices but using MatRingElem instead without really considering whether the operation/test makes sense in this context.

[JohnAAbbott]

Another oddity: we have the names MatElem and MatRingElem but also MatrixGroupElem (rather than MatGroupElem). We do also have MatrixElem but that is a union... the one that triggered some problems.
I had hoped to use Julia's methodswith to find out what one can do with a MatRingElem and what one can do with a MatrixGroupElem... unfortunately the results were less helpful than hoped (e.g. since many functions which can take MatRingElem were not listed, and asking about MatrixElem was also less helpful than hoped).

[JohnAAbbott]

What is the utility of the file AbstractAlegbra/src/julia/Matrix.jl? [edited 2025-12-05 CET 16:07]

The first comment states:

  Matrix.jl : Additional AbstractAlgebra functionality for Julia Matrices

Surely the OSCAR specific matrix types will handle matrices of OSCAR values better than Julia's native matrices, no?

In src/Matrix.jl near lines 16--62
Why do we want to facilitate converting an OSCAR MatElem or MatRingElem to a Julia matrix? Is there a (sane) use-case for this?

In src/Matrix.jl near lines 69-100
Why do we want to implement swap_rows & swap_cols for Julia matrices?

[thofma]

What is the utility of the file AbstractAlegbra/src/julia/Matrix.jl? [edited: 2025-12-05 CET 16:07]

The first comment states:

  Matrix.jl : Additional AbstractAlgebra functionality for Julia Matrices

It does not for me.

Surely the OSCAR specific matrix types will handle matrices of OSCAR values better than Julia's native matrices, no?

No, not in all cases. It depends on what "handle" means. A few algorithms are much faster with plain julia matrices if one uses them as plain two-dimensional arrays without any "matrix functionality". (IMHO they should not have been called "Matrix" in the first place.)

In src/Matrix.jl near lines 69-100 Why do we want to implement swap_rows & swap_cols for Julia matrices?

Have you checked the git history? Someone must have added it and the reason might be given in the commit message. But also I am not sure how relevant this is for this PR here?

P.S.: When you used methodswith, have you used ; supertypes = true? I found it quite useless for most input without setting this to true.

[thofma]

Which of the questions you asked (concerning the PR here) are still open? It seems there were some offline discussions, which might have answered some (all?) of those questions?

[JohnAAbbott]

What is the utility of the file AbstractAlegbra/src/Matrix.jl?
The first comment states:

  Matrix.jl : Additional AbstractAlgebra functionality for Julia Matrices

It does not for me.

Surely the OSCAR specific matrix types will handle matrices of OSCAR values better than Julia's native matrices, no?

No, not in all cases. It depends on what "handle" means. A few algorithms are much faster with plain julia matrices if one uses them as plain two-dimensional arrays without any "matrix functionality". (IMHO they should not have been called "Matrix" in the first place.)

In src/Matrix.jl near lines 69-100 Why do we want to implement swap_rows & swap_cols for Julia matrices?

Have you checked the git history? Someone must have added it and the reason might be given in the commit message. But also I am not sure how relevant this is for this PR here?

P.S.: When you used methodswith, have you used ; supertypes = true? I found it quite useless for most input without setting this to true.

Sorry wrong file path: it should have been src/julia/Matrix.jl
Thanks for the hint about supertypes!
git blame reports @fingolfin (code) and @lgoettgens (doc) as the main contributors; changes made around 2023-09-xx

[fingolfin]

This has conflicts with master.

To repeat what I said earlier but in writing: I strongly recommend to suppress the urge to complicate this with removal of functionality (which is a breaking change). At least minimize such changes

[JohnAAbbott]

I have just pushed some corrections: all AA tests passed on my computer. I'll wait to see what GitHub says...

[codecov]

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 88.00%. Comparing base (a68dfd3) to head (f435653).
⚠️ Report is 16 commits behind head on master.

Additional details and impacted files

@@           Coverage Diff           @@
##           master    #2207   +/-   ##
=======================================
  Coverage   88.00%   88.00%           
=======================================
  Files         127      127           
  Lines       31795    31758   -37     
=======================================
- Hits        27980    27950   -30     
+ Misses       3815     3808    -7     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

[lgoettgens]

This is going in the right direction, and I am happy about how much stuff you were just able to replace by a one-liner. A few comments below, that will help with type stability and future maintainability

[lgoettgens]

just noticed that I have some forgotten comments. If you already fixed them in the meantime, please just ignore

[lgoettgens]

In case you didn't notice: There are some more comments that github decided to hide from you

[JohnAAbbott]

In case you didn't notice: There are some more comments that github decided to hide from you...

I had overlooked them. Now I have responded.

[fingolfin]

@JohnAAbbott I marked several of my comments as "resolved", so that those which remain are more visible :-)

[JohnAAbbott]

What next? As far as I can see the code is now working (except for 1 failing check -- anyone know what that is about?)
This PR was triggered by an ambiguity in is_square, and it was suggested that is_square for MatRingElem should be removed (right?). Removing this function means that several functions which operate on MatrixElem will have to be split into two: one where we simply change the argument type to MatElem, and a new "one-liner" for MatRingElem which simply calls the MatElem version on the one data field inside the MatRingElem -- this does imply that some function calls will "wastefully" check that the given matrix is square, but that surely has negligible cost.
Question: Should we merge this PR, and then start a new one containing all the changes outlined above (namely: remove is_square and split all functions which have to be split as a consequence)? I wonder if this will imply that some functions should be moved from one source code file to another?

[JohnAAbbott]

I think it is worth merging the current code, and outstanding matters can be handled in a new PR (to avoid making this PR still longer -- see also my last comment in Conversation).

[thofma]

Agreed, lets get this in.