Skip to content

Split promote methods for MatrixElem#2420

Open
lgoettgens wants to merge 2 commits into
Nemocas:masterfrom
lgoettgens:lg/MatrixElem-promote
Open

Split promote methods for MatrixElem#2420
lgoettgens wants to merge 2 commits into
Nemocas:masterfrom
lgoettgens:lg/MatrixElem-promote

Conversation

@lgoettgens

Copy link
Copy Markdown
Member

Split off from #2280. This currently also contains #2419 since I get a bunch of ambiguity issues without it.

One could think that this is breaking due to removing the dispatch for Base.promote(x::MatSpaceElem{S}, ::MatRingElem{T}) (and the other way around). But this is not the case, as this dispatch used to run into an ambiguity error:

julia> R = matrix_ring(ZZ, 3);

julia> S = matrix_space(QQ, 3, 3);

julia> Base.promote(one(R), one(S)) # for different type parameters
ERROR: MethodError: promote(::AbstractAlgebra.Generic.MatRingElem{BigInt}, ::AbstractAlgebra.Generic.MatSpaceElem{Rational{BigInt}}) is ambiguous.

Candidates:
  promote(x::MatrixElem{S}, y::MatrixElem{T}) where {S<:NCRingElement, T<:NCRingElement}
    @ AbstractAlgebra ~/code/julia/AbstractAlgebra.jl/src/Matrix.jl:1178
  promote(x::S, y::MatElem{T}) where {S<:NCRingElement, T<:NCRingElement}
    @ AbstractAlgebra ~/code/julia/AbstractAlgebra.jl/src/Matrix.jl:1235

Possible fix, define
  promote(::S, ::MatElem{T}) where {S<:NCRingElement, S<:MatRingElem{S}, T<:NCRingElement}

julia> S = matrix_space(ZZ, 3, 3);

julia> Base.promote(one(R), one(S)) # for the same type parameter
ERROR: MethodError: promote(::AbstractAlgebra.Generic.MatRingElem{BigInt}, ::AbstractAlgebra.Generic.MatSpaceElem{BigInt}) is ambiguous.

Candidates:
  promote(x::MatrixElem{S}, y::MatrixElem{T}) where {S<:NCRingElement, T<:NCRingElement}
    @ AbstractAlgebra ~/code/julia/AbstractAlgebra.jl/src/Matrix.jl:1178
  promote(x::S, y::MatElem{T}) where {S<:NCRingElement, T<:NCRingElement}
    @ AbstractAlgebra ~/code/julia/AbstractAlgebra.jl/src/Matrix.jl:1235

Possible fix, define
  promote(::S, ::MatElem{T}) where {S<:NCRingElement, S<:MatRingElem{S}, T<:NCRingElement}

With this PR, the behavior is as follows:

julia> R = matrix_ring(ZZ, 3);

julia> S = matrix_space(QQ, 3, 3);

julia> Base.promote(one(R), one(S)) # for different type parameters
ERROR: Cannot promote to common type
Stacktrace:
 [1] error(s::String)
   @ Base ./error.jl:44
 [2] promote(x::AbstractAlgebra.Generic.MatSpaceElem{Rational{BigInt}}, y::AbstractAlgebra.Generic.MatRingElem{BigInt})
   @ AbstractAlgebra ~/code/julia/AbstractAlgebra.jl/src/Matrix.jl:1231
 [3] promote(x::AbstractAlgebra.Generic.MatRingElem{BigInt}, y::AbstractAlgebra.Generic.MatSpaceElem{Rational{BigInt}})
   @ AbstractAlgebra ~/code/julia/AbstractAlgebra.jl/src/Matrix.jl:1236
 [4] top-level scope
   @ REPL[57]:1

julia> S = matrix_space(ZZ, 3, 3);

julia> Base.promote(one(R), one(S)) # for the same type parameter
([1 0 0; 0 1 0; 0 0 1], [[1 0 0; 0 1 0; 0 0 1] [0 0 0; 0 0 0; 0 0 0] [0 0 0; 0 0 0; 0 0 0]; [0 0 0; 0 0 0; 0 0 0] [1 0 0; 0 1 0; 0 0 1] [0 0 0; 0 0 0; 0 0 0]; [0 0 0; 0 0 0; 0 0 0] [0 0 0; 0 0 0; 0 0 0] [1 0 0; 0 1 0; 0 0 1]])

I am not sure if we want to keep the last line like that. What happens is that the coefficient ring of the matrix space gets extended to the matrix ring, mapping the original coefficients to diagonal matrices.
However, we have to be careful with adding explicit errors here to not mess up with genuine use-cases where people use MatRingElems as coefficients in their matrices.

@codecov

codecov Bot commented Jun 8, 2026

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 98.46154% with 1 line in your changes missing coverage. Please review.
✅ Project coverage is 88.14%. Comparing base (c982ae7) to head (8984946).

Files with missing lines Patch % Lines
src/MatRing.jl 97.87% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##           master    #2420      +/-   ##
==========================================
+ Coverage   88.12%   88.14%   +0.01%     
==========================================
  Files         130      130              
  Lines       32955    33002      +47     
==========================================
+ Hits        29043    29088      +45     
- Misses       3912     3914       +2     

☔ View full report in Codecov by Harness.
📢 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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant