Change or split ==, isequal and prmote taking MatrixElem arguments

[JohnAAbbott]

First phase of splitting functions which take MatrixElem

[fieker]

On Fri, Jan 09, 2026 at 07:18:02AM -0800, Max Horn wrote: @fingolfin commented on this pull request. > +function canonical_unit(a::MatElem{T}) where T <: NCRingElement + @Req (nrows(a) > 0 && ncols(a) > 0) "Matrix must contain at least one entry" + return canonical_unit(a[1, 1]) +end ```suggestion canonical_unit(a::MatElem) = canonical_unit(matrix(a)) ```

a[1,1] was wrong anyway, it should be the 1st (in some fixed ordering) non-zero entry of a

…

-- Reply to this email directly or view it on GitHub: #2280 (review) You are receiving this because you are subscribed to this thread. Message ID: ***@***.***>

[JohnAAbbott]

I marked several of @fingolfin suggestions as resolved because I already had some of the changes in my local copy. The latest push should resolve all of them.

[fingolfin]

Test fails:

Matrix.conversion: Test Failed at /Users/runner/work/AbstractAlgebra.jl/AbstractAlgebra.jl/test/Matrix-test.jl:136
  Expression: matrix(U, Ra) == Ra
   Evaluated: [1 2; t^2 t-1] == [1 2; t^2 t-1]

I think it is comparing a MatRingElem and a MatElem and getting an unexpected result. I assume (but have not checked) that it used to get true; and it now gets false. That's bad: it really should be either true (so a missing == would have to be (re-)added), or an exception (then the test needs to be adjusted).

I'd be OK with either, but false is not acceptable

[JohnAAbbott]

One failing test: in test/Matrix-test.jl near line 136 there is the test matrix(U,Ra) == Ra where Ra is a MatRingElem and U is its base ring. The test fails because we are comparing a MatElem with a MatRingElem, and this gives false.
What is the preferred course of action?

1. Remove the failing test
2. Modify the code so that comparison of a MatElem with a MatRingElem compares the sizes and the entries
3. Other?

[JohnAAbbott]

Test fails:

Matrix.conversion: Test Failed at /Users/runner/work/AbstractAlgebra.jl/AbstractAlgebra.jl/test/Matrix-test.jl:136
  Expression: matrix(U, Ra) == Ra
   Evaluated: [1 2; t^2 t-1] == [1 2; t^2 t-1]

I think it is comparing a MatRingElem and a MatElem and getting an unexpected result. I assume (but have not checked) that it used to get true; and it now gets false. That's bad: it really should be either true (so a missing == would have to be (re-)added), or an exception (then the test needs to be adjusted).

I'd be OK with either, but false is not acceptable

The equality test being called is likely unintentional:

==(x::MatElem, y::NCRingElem)
     @ AbstractAlgebra ~/OSCAR/GIT/AbstractAlgebra.jl/src/Matrix.jl:1252

It calls promote which changes the entries of the MatElem into MatRingElem values; that is surely unintentional!

[codecov]

Codecov Report

❌ Patch coverage is 91.80328% with 5 lines in your changes missing coverage. Please review.
✅ Project coverage is 88.11%. Comparing base (032b699) to head (b0bfdaa).

Files with missing lines	Patch %	Lines
src/Matrix.jl	76.19%	5 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #2280      +/-   ##
==========================================
- Coverage   88.15%   88.11%   -0.05%     
==========================================
  Files         126      126              
  Lines       31999    32038      +39     
==========================================
+ Hits        28210    28231      +21     
- Misses       3789     3807      +18     

☔ 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.

[JohnAAbbott]

I have added new equality test functions (just the == version, not isequal) to cover comparison of a MatElem and a MatRingElem; currently the implementation just compares the underlying matrix value, but it could easily generate an error instead.
There's a catch: the new functions have introduced an ambiguity:

  Test threw exception
  Expression: S(m) == m
  MethodError: ==(::AbstractAlgebra.Generic.MatSpaceElem{AbstractAlgebra.Generic.MatRingElem{BigInt}}, ::AbstractAlgebra.Generic.MatRingElem{BigInt}) is ambiguous.
  
  Candidates:
    ==(x::MatElem{S}, y::MatRingElem{T}) where {S<:NCRingElement, T<:NCRingElement}
      @ AbstractAlgebra ~/OSCAR/GIT/AbstractAlgebra.jl/src/MatRing.jl:266
    ==(x::MatrixElem{T}, y::T) where T<:NCRingElem
      @ AbstractAlgebra ~/OSCAR/GIT/AbstractAlgebra.jl/src/Matrix.jl:1386
  
  Possible fix, define
    ==(::MatElem{T}, ::MatRingElem{T}) where {T<:NCRingElem, T<:NCRingElement}

I had wanted to use NCRingElem rather than NCRingElement, but that does not work with matrices over ZZ running in an AbstractAlgebra context (rather than Oscar). Frankly, I find it questionable to have a function with signature:

  ==(x::MatrixElem{T}, y::T) where T<:NCRingElem
      @ AbstractAlgebra ~/OSCAR/GIT/AbstractAlgebra.jl/src/Matrix.jl:1386

How often does it arise that one wants to know if a matrix is a scalar multiple of a truncated identity matrix? We already have tests for is_zero and is_identity which presumably cover the common cases.

[fingolfin]

How often does it arise that one wants to know if a matrix is a scalar multiple of a truncated identity matrix? We already have tests for is_zero and is_identity which presumably cover the common cases.

Personally I agree, and I'd be supportive of efforts trying to change that, but:

1. it may be more wide spread than you think, e.g. people doing bla == 0 instead of is_zero(bla) is done in a lot of places (I am not happy about it, but it's a fact)
2. if we remove this, we must be careful to make sure the result in affected code is an error (that we then fix) and not a silently wrong comparison
3. it should not be done in this PR, but in a separate one; perhaps open an issue for now as a reminder to look again into it later on

[JohnAAbbott]

1. it may be more wide spread than you think, e.g. people doing `bla == 0` instead of `is_zero(bla)` is done in a lot of places (I am not happy about it, but it's a fact)

Ahhh! That's a fair point.

[JohnAAbbott]

To make the tests pass I disabled two checks because they triggered the ambiguity error. Shall I leave them commented out?

[JohnAAbbott]

Here's another surprise. I wanted to increase code coverage (to keep GitHub happy), and discovered that when M is a AbstractAlgebra.Generic.MatSpaceElem{BigInt} then the call mul!(M,M,M) does not call function mul!(c::T, a::T, b::T) where T <: MatElem which is defined in src/Matrix.jl but instead calls another function...
Indeed there are the following functions in src/generic/Matrix.jl near lines 114--128

function AbstractAlgebra.add!(A::Mat{T}, B::Mat{T}, C::Mat{T}) where T
  A.entries .= B.entries .+ C.entries
  return A
end

function AbstractAlgebra.sub!(A::Mat{T}, B::Mat{T}, C::Mat{T}) where T
  A.entries .= B.entries .- C.entries
  return A
end

function AbstractAlgebra.mul!(A::Mat{T}, B::Mat{T}, C::Mat{T}) where T
  A.entries .= (B * C).entries
  return A
end

To clarify: in src/generic/GenericTypes.jl there is the definition: abstract type Mat{T} <: MatElem{T} end
Please advise!

[lgoettgens]

Here's another surprise. I wanted to increase code coverage (to keep GitHub happy), and discovered that when M is a AbstractAlgebra.Generic.MatSpaceElem{BigInt} then the call mul!(M,M,M) does not call function mul!(c::T, a::T, b::T) where T <: MatElem which is defined in src/Matrix.jl but instead calls another function... Indeed there are the following functions in src/generic/Matrix.jl near lines 114--128

function AbstractAlgebra.add!(A::Mat{T}, B::Mat{T}, C::Mat{T}) where T
  A.entries .= B.entries .+ C.entries
  return A
end

function AbstractAlgebra.sub!(A::Mat{T}, B::Mat{T}, C::Mat{T}) where T
  A.entries .= B.entries .- C.entries
  return A
end

function AbstractAlgebra.mul!(A::Mat{T}, B::Mat{T}, C::Mat{T}) where T
  A.entries .= (B * C).entries
  return A
end

To clarify: in src/generic/GenericTypes.jl there is the definition: abstract type Mat{T} <: MatElem{T} end Please advise!

The functions in src/Matrix.jl may not be testable in AA alone, but may need Nemo to be loaded (which we don't do in the CI here). Thus, I would say there is nothing you can do here

[lgoettgens]

These should either work or (if they are logical nonsense) be removed. Just commenting out is bad.

[JohnAAbbott]

They do currently work, but the point I wanted to make earlier is whether we really want to guarantee that they will work evermore. I think we should not make this guarantee, and @fingolfin wrote Personally I agree. So based on that comment I shall delete these tests since they do not represent behaviour that we wish to guarantee in the future.

[fingolfin]

@JohnAAbbott I think you misunderstood me here: I agree that we should work on removing this feature. But that's not done by just removing the test, or the code. Rather, we have to carefully do this, to avoid breaking people's code badly. To be explicitly:

1. such a thing should never be done as an aside in PR that is focused on something else (where it will slip by the attention of people)
2. rather, deprecating a feature should be done in its own dedicated PR, so that people can be aware of it
3. it should be made sure that it is mentioned as a breaking change in the release notes (another reason why having it in its own PR is useful: we can then use our existing tooling and edit the PR title to be directly suitable for inclusion in the release notes)
4. the change should be done in a way that minimizes "hidden" breakage. In the case at hand, this means that it would be OK from "comparison works in a sensible way" to "comparison raises an helpful error. It would not be OK to go to "comparison suddenly always returns false" or "comparison raises an unhelpful error".
5. this change then of course would have to be reflected in the tests -- but not by just removing a test, but by replacing it with a test that verifies the new property (e.g. "always raises helpful error") actually holds

My points 1+2 also apply to other "unrelated" changes. This is why e.g. I said the change to how empty matrices are printed should not be discussed via a code comment inserted here, but in a separate issue; and be resolved in a separate PR, dedicated to that.

If juggling multiple git branches is a hold-up factor here, I'd be happy to sit with you to discuss some ways how you can manage multiple active git branches with low friction.

[JohnAAbbott]

I have marked this as Ready for review because the CI checks all pass. Note that passing was achieved by disabling a whole test-set in generic/Matrix-test.jl which was testing behaviour which we now doubt that we should guarantee (e.g. see @fingolfin comment above containing Personally I agree)
A tricky point was equality testing of a MatElem with a MatRingElem: this could be "ambiguous" if the MatElem has entries from a MatRing. However, some people may have written code such as M == 0 instead of is_zero(M) because there is an == operator which accepts a matrix and an element of a ring... Discussion about this will be held in a separate issue (yet to be created).

[JohnAAbbott]

I have just created issue #2299 to discuss the issue related to equality testing. If the change there is approved then the implementation will be in a separate PR.

[lgoettgens]

I have found a minimal diff that preserves the current behavior (which I'll commit here in a minute). This does not intend to force a decision on #2299, but it allows for this PR to continue on in a non-breaking way.
Depending on the outcome of #2299, we might remove the touched methods in a future (breaking) PR again.

[JohnAAbbott]

I shall let @lgoettgens take over this PR from now on.

[lgoettgens]

I shall let @lgoettgens take over this PR from now on.

As a first step, I cherry-picked the (in my opinion) uncontroversial changes into the new PR #2302. Once that is in, I'll rebase this one here to then have something smaller to reason about. If I am not mistaken, there should be only promote and == left then.

[fingolfin]

This line could be merged right away

[fingolfin]

Could be merged right away

[fingolfin]

Could be merged right away

[thofma]

Is this good to go? Still marked as draft and there are some unaddressed(?) comments by @fingolfin

[fingolfin]

If we do this, we must also change the hash method for MatRingElem.

Which probably is advisable anyway, the current one is completely custom:

function Base.hash(a::MatRingElem, h::UInt)
   b = 0x6413942b83a26c65%UInt
   for i in 1:nrows(a)
      for j in 1:ncols(a)
         b = xor(b, xor(hash(a[i, j], h), h))
         b = (b << 1) | (b >> (sizeof(Int)*8 - 1))
      end
   end
   return b
end

this should be changed to

Base.hash(a::MatRingElem, h::UInt) = hash(matrix(a), h)

[fingolfin]

@JohnAAbbott please update this, with my comments taken into account, so that we can discuss it on Wednesday during triage.

[JohnAAbbott]

I can try, but I thought that @lgoettgens had taken over the PR.

[lgoettgens]

I split this into a few more pieces that feel easier to digest to me: #2419, #2420, #2421
These each have a comment with how breaking they are and questions / edge cases for triage to discuss.

As everything from this PR should appear in one of these three, I will close this now.