AbstractAlgebra.jl icon indicating copy to clipboard operation
AbstractAlgebra.jl copied to clipboard

Change `MatRing` & `MatRingElem` to wrap a "native" matrix, not a `Matrix{T}`

Open fingolfin opened this issue 11 months ago • 3 comments

I think we should change the implementation of Generic.MatRingElem from

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

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

to something more like this (untested, but I hope at least the principle idea gets across)

struct MatRingElem{T <: NCRingElement} <: AbstractAlgebra.MatRingElem{T}
   data::MatElem{T}

   function MatRingElem{T}(A::MatElem{T}) where T <: NCRingElement
      @assert A isa dense_matrix_type(T)
      return new{T}(A)
   end
end

function data(M::MatRingElem{T}) where T
  return M.data::dense_matrix_type(T)
end

This way those MatRingElem are less useless / more efficient. After a quick glance a bunch of code should actually become simpler. And some code that currently is implement for Generic.MatRingElem and Generic.Mat simultaneously needs to be untangled, but that's fine, e.g.

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

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

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

Base.@propagate_inbounds function setindex!(a::Union{Mat, MatRingElem}, 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)

fingolfin avatar Jan 13 '25 23:01 fingolfin

Note that we define

const MatrixElem{T} = Union{MatElem{T}, MatRingElem{T}}

and therefore when doing this untangling we need to inspect a bunch of methods taking an MatrixElem to see if they should perhaps be restricted to just MatElem.

fingolfin avatar Feb 14 '25 13:02 fingolfin

In fact once we have untangled this, we have a very neat and efficient way to go from a MatRingElem to its "underlying" MatElem, and then can use that to avoid most if not all uses of MatrixElem in AbstractAlgebra and elsewhere.

CC @JohnAAbbott

fingolfin avatar Nov 05 '25 09:11 fingolfin

I agree, we should change this.

thofma avatar Nov 05 '25 10:11 thofma

See PR #2207 for a start on this, but it needs to be finished (CC @JohnAAbbott )

fingolfin avatar Nov 19 '25 10:11 fingolfin

From triage: @JohnAAbbott hopes to finish this today.

mjrodgers avatar Dec 03 '25 11:12 mjrodgers