Skip to content

Rewrite MatRing to wrap a native matrix #2207

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
fingolfin merged 19 commits into from
Dec 12, 2025
Merged

Rewrite MatRing to wrap a native matrix #2207

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
19 commits
Select commit Hold shift + click to select a range
d0efdf4
WIP: rewrite MatRing to wrap a native matrix
fingolfin Dec 21, 2024
06c5bab
fixed code so that tests pass
JohnAAbbott Dec 8, 2025
1103e30
Merge branch 'master' into mh/MatRing-native-matrix
JohnAAbbott Dec 8, 2025
3bbe379
Update src/NCPoly.jl
JohnAAbbott Dec 8, 2025
1f97847
Update src/generic/MatRing.jl
JohnAAbbott Dec 8, 2025
9369964
Update src/generic/MatRing.jl
JohnAAbbott Dec 8, 2025
da2d2ea
Update src/generic/MatRing.jl
JohnAAbbott Dec 8, 2025
e5a24d4
Update src/generic/MatRing.jl
JohnAAbbott Dec 8, 2025
7437ac0
Update src/generic/GenericTypes.jl
JohnAAbbott Dec 8, 2025
74045d8
Revision following lgoettgens suggestions
JohnAAbbott Dec 8, 2025
15c8c9a
Replaced field accessors by accessor-function calls
JohnAAbbott Dec 9, 2025
b379297
indentation
JohnAAbbott Dec 9, 2025
12ff76f
Minor cleaning - thanks to thofma
JohnAAbbott Dec 9, 2025
603a2a6
Update src/NCPoly.jl
lgoettgens Dec 10, 2025
3e114bd
Use macro @req
JohnAAbbott Dec 10, 2025
3ef99ac
Removed cruft
JohnAAbbott Dec 10, 2025
5d734bd
Revised add! according to fingolfin's suggestion
JohnAAbbott Dec 10, 2025
677d2d3
New signatures for + and - following fingolfin suggestion
JohnAAbbott Dec 10, 2025
f435653
Merge branch 'mh/MatRing-native-matrix' of github.com:Nemocas/Abstrac…
JohnAAbbott Dec 10, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
22 changes: 0 additions & 22 deletions src/MatRing.jl
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -180,28 +180,6 @@ function show(io::IO, a::MatRing)
end
end

###############################################################################
#
# Binary operations
#
###############################################################################

function *(x::MatRingElem{T}, y::MatRingElem{T}) where {T <: NCRingElement}
degree(x) != degree(y) && error("Incompatible matrix degrees")
A = similar(x)
C = base_ring(x)()
for i = 1:nrows(x)
for j = 1:ncols(y)
A[i, j] = base_ring(x)()
for k = 1:ncols(x)
C = mul!(C, x[i, k], y[k, j])
A[i, j] = add!(A[i, j], C)
end
end
end
return A
end

###############################################################################
#
# Ad hoc comparisons
Expand Down
8 changes: 5 additions & 3 deletions src/Matrix.jl
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -823,7 +823,7 @@ end
#
###############################################################################

function +(x::MatrixElem{T}, y::MatrixElem{T}) where {T <: NCRingElement}
function +(x::T, y::T) where {T <: MatrixElem}
check_parent(x, y)
r = similar(x)
for i = 1:nrows(x)
Expand All @@ -834,7 +834,7 @@ function +(x::MatrixElem{T}, y::MatrixElem{T}) where {T <: NCRingElement}
return r
end

function -(x::MatrixElem{T}, y::MatrixElem{T}) where {T <: NCRingElement}
function -(x::T, y::T) where {T <: MatrixElem}
check_parent(x, y)
r = similar(x)
for i = 1:nrows(x)
Expand All @@ -845,7 +845,7 @@ function -(x::MatrixElem{T}, y::MatrixElem{T}) where {T <: NCRingElement}
return r
end

function *(x::MatElem{T}, y::MatElem{T}) where {T <: NCRingElement}
function *(x::T, y::T) where {T <: MatrixElem}
Copy link
Member Author

@fingolfin fingolfin Dec 3, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Collaborator

@JohnAAbbott JohnAAbbott Dec 10, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

ncols(x) != nrows(y) && error("Incompatible matrix dimensions")
base_ring(x) !== base_ring(y) && error("Base rings do not match")
A = similar(x, nrows(x), ncols(y))
Expand Down Expand Up @@ -2628,6 +2628,8 @@ function minors_iterator(M::MatElem, k::Int)
return (det(M[rows, cols]) for rows in row_indices for cols in col_indices)
end

# TODO: how to get the 'positions' of the minors?

@doc raw"""
minors_iterator_with_position(A::MatElem, k::Int)

Expand Down
15 changes: 5 additions & 10 deletions src/generic/GenericTypes.jl
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1117,22 +1117,17 @@ struct MatRing{T <: NCRingElement} <: AbstractAlgebra.MatRing{T}
end

struct MatRingElem{T <: NCRingElement} <: AbstractAlgebra.MatRingElem{T}
base_ring::NCRing
entries::Matrix{T}
data::MatElem{T}

function MatRingElem{T}(R::NCRing, A::Matrix{T}) where T <: NCRingElement
@assert elem_type(R) === T
return new{T}(R, A)
function MatRingElem{T}(A::MatElem{T}) where T <: NCRingElement
return new{T}(A)
end
end

function MatRingElem{T}(R::NCRing, n::Int, A::Vector{T}) where T <: NCRingElement
@assert elem_type(R) === T
t = Matrix{T}(undef, n, n)
for i = 1:n, j = 1:n
t[i, j] = A[(i - 1) * n + j]
end
return MatRingElem{T}(R, t)
t = matrix(R, n, n, A)
return MatRingElem{T}(t)
end

###############################################################################
Expand Down
101 changes: 39 additions & 62 deletions src/generic/MatRing.jl
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,27 +16,46 @@ elem_type(::Type{MatRing{T}}) where {T <: NCRingElement} = MatRingElem{T}

base_ring(a::MatRing{T}) where {T <: NCRingElement} = a.base_ring::parent_type(T)

base_ring(a::MatRingElem{T}) where {T <: NCRingElement} = base_ring(a.data)

@doc raw"""
parent(a::MatRingElem{T}) where T <: NCRingElement

Return the parent object of the given matrix.
"""
parent(a::MatRingElem{T}) where T <: NCRingElement = MatRing{T}(base_ring(a), size(a.entries)[1])
parent(a::MatRingElem{T}) where T <: NCRingElement = MatRing{T}(base_ring(a), nrows(a.data))

is_exact_type(::Type{MatRingElem{T}}) where T <: NCRingElement = is_exact_type(T)

is_domain_type(::Type{MatRingElem{T}}) where T <: NCRingElement = false

###############################################################################
#
# Basic manipulation
#
###############################################################################

number_of_rows(a::MatRingElem) = nrows(a.data)

number_of_columns(a::MatRingElem) = ncols(a.data)

Base.@propagate_inbounds getindex(a::MatRingElem, r::Int, c::Int) = a.data[r, c]

Base.@propagate_inbounds function setindex!(a::MatRingElem, d::NCRingElement,
r::Int, c::Int)
a.data[r, c] = base_ring(a)(d)
end

Base.isassigned(a::MatRingElem, i, j) = isassigned(a.data, i, j)

###############################################################################
#
# Transpose
#
###############################################################################

function transpose(x::MatRingElem{T}) where T <: NCRingElement
arr = permutedims(x.entries)
z = MatRingElem{T}(base_ring(x), arr)
return z
return MatRingElem{T}(transpose(x.data))
end

###############################################################################
Expand All @@ -48,18 +67,18 @@ end
function _can_solve_with_solution_lu(M::MatRingElem{T}, B::MatRingElem{T}) where {T <: RingElement}
check_parent(M, B)
R = base_ring(M)
MS = MatSpaceElem{T}(R, M.entries) # convert to ordinary matrix
BS = MatSpaceElem{T}(R, B.entries)
MS = M.data # TODO: is this right?
BS = B.data # TODO: is this right?
flag, S = _can_solve_with_solution_lu(MS, BS)
SA = MatRingElem{T}(R, S.entries)
SA = MatRingElem{T}(S)
return flag, SA
end

function AbstractAlgebra.can_solve_with_solution(M::MatRingElem{T}, B::MatRingElem{T}) where {T <: RingElement}
check_parent(M, B)
R = base_ring(M)
MS = MatSpaceElem{T}(R, M.entries) # convert to ordinary matrix
BS = MatSpaceElem{T}(R, B.entries)
MS = M.data # TODO: is this right?
BS = B.data # TODO: is this right?
flag, S = can_solve_with_solution(MS, BS)
SA = MatRingElem{T}(R, S.entries)
return flag, SA
Expand All @@ -68,10 +87,10 @@ end
function _can_solve_with_solution_fflu(M::MatRingElem{T}, B::MatRingElem{T}) where {T <: RingElement}
check_parent(M, B)
R = base_ring(M)
MS = MatSpaceElem{T}(R, M.entries) # convert to ordinary matrix
BS = MatSpaceElem{T}(R, B.entries)
MS = M.data # convert to ordinary matrix
BS = B.data
flag, S, d = _can_solve_with_solution_fflu(MS, BS)
SA = MatRingElem{T}(R, S.entries)
SA = MatRingElem{T}(S)
return flag, SA, d
end

Expand All @@ -88,8 +107,7 @@ Return the minimal polynomial $p$ of the matrix $M$. The polynomial ring $S$
of the resulting polynomial must be supplied and the matrix must be square.
"""
function minpoly(S::Ring, M::MatRingElem{T}, charpoly_only::Bool = false) where {T <: RingElement}
MS = MatSpaceElem{T}(base_ring(M), M.entries) # convert to ordinary matrix
return minpoly(S, MS, charpoly_only)
return minpoly(S, M.data, charpoly_only)
end

function minpoly(M::MatRingElem{T}, charpoly_only::Bool = false) where {T <: RingElement}
Expand All @@ -105,7 +123,7 @@ end
###############################################################################

function add!(A::MatRingElem{T}, B::MatRingElem{T}) where T <: NCRingElement
A.entries .+= B.entries
A.data = add!(A.data, B.data)
return A
end

Expand All @@ -129,48 +147,20 @@ end

function (a::MatRing{T})() where {T <: NCRingElement}
R = base_ring(a)
entries = Matrix{T}(undef, a.n, a.n)
for i = 1:a.n
for j = 1:a.n
entries[i, j] = zero(R)
end
end
z = MatRingElem{T}(R, entries)
z = MatRingElem{T}(zero_matrix(R, a.n, a.n))
return z
end

function (a::MatRing{T})(b::S) where {S <: NCRingElement, T <: NCRingElement}
R = base_ring(a)
entries = Matrix{T}(undef, a.n, a.n)
rb = R(b)
for i = 1:a.n
for j = 1:a.n
if i != j
entries[i, j] = zero(R)
else
entries[i, j] = rb
end
end
end
z = MatRingElem{T}(R, entries)
z = MatRingElem{T}(scalar_matrix(R, a.n, R(b)))
return z
end

# to resolve ambiguity for MatRing{MatRing{...}}
function (a::MatRing{T})(b::T) where {S <: NCRingElement, T <: MatRingElem{S}}
R = base_ring(a)
entries = Matrix{T}(undef, a.n, a.n)
rb = R(b)
for i = 1:a.n
for j = 1:a.n
if i != j
entries[i, j] = zero(R)
else
entries[i, j] = rb
end
end
end
z = MatRingElem{T}(R, entries)
z = MatRingElem{T}(scalar_matrix(R, a.n, R(b)))
return z
end

Expand All @@ -182,33 +172,20 @@ end
function (a::MatRing{T})(b::MatrixElem{S}) where {S <: NCRingElement, T <: NCRingElement}
R = base_ring(a)
_check_dim(nrows(a), ncols(a), b)
entries = Matrix{T}(undef, nrows(a), ncols(a))
for i = 1:nrows(a)
for j = 1:ncols(a)
entries[i, j] = R(b[i, j])
end
end
z = MatRingElem{T}(R, entries)
z = MatRingElem{T}(matrix(R, b))
return z
end

function (a::MatRing{T})(b::Matrix{S}) where {S <: NCRingElement, T <: NCRingElement}
R = base_ring(a)
_check_dim(a.n, a.n, b)
entries = Matrix{T}(undef, a.n, a.n)
for i = 1:a.n
for j = 1:a.n
entries[i, j] = R(b[i, j])
end
end
z = MatRingElem{T}(R, entries)
z = MatRingElem{T}(matrix(R, b))
return z
end

function (a::MatRing{T})(b::Vector{S}) where {S <: NCRingElement, T <: NCRingElement}
_check_dim(a.n, a.n, b)
b = Matrix{S}(transpose(reshape(b, a.n, a.n)))
z = a(b)
z = MatRingElem{T}(matrix(R, a.n, a.n, b))
return z
end

Expand Down
10 changes: 5 additions & 5 deletions src/generic/Matrix.jl
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -43,18 +43,18 @@ dense_matrix_type(::Type{T}) where T <: NCRingElement = MatSpaceElem{T}
#
###############################################################################

number_of_rows(a::Union{Mat, MatRingElem}) = size(a.entries, 1)
number_of_rows(a::Mat) = size(a.entries, 1)

number_of_columns(a::Union{Mat,MatRingElem}) = size(a.entries, 2)
number_of_columns(a::Mat) = size(a.entries, 2)

Base.@propagate_inbounds getindex(a::Union{Mat, MatRingElem}, r::Int, c::Int) = a.entries[r, c]
Base.@propagate_inbounds getindex(a::Mat, r::Int, c::Int) = a.entries[r, c]

Base.@propagate_inbounds function setindex!(a::Union{Mat, MatRingElem}, d::NCRingElement,
Base.@propagate_inbounds function setindex!(a::Mat, d::NCRingElement,
r::Int, c::Int)
a.entries[r, c] = base_ring(a)(d)
end

Base.isassigned(a::Union{Mat,MatRingElem}, i, j) = isassigned(a.entries, i, j)
Base.isassigned(a::Mat, i, j) = isassigned(a.entries, i, j)

################################################################################
#
Expand Down
Loading