Skip to content
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

Jump to bottom

Change similar, zero_matrix, ones_matrix to use UndefInitializer constructors #1909

Open
wants to merge 7 commits into
base: master
Choose a base branch
from
Open

Change similar, zero_matrix, ones_matrix to use UndefInitializer constructors #1909

Show file tree
Hide file tree
Changes from 4 commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
ffa3842
Change some matrix function to use UndefInitializer constructors
fingolfin Nov 19, 2024
1a3012f
Delegate more from MatSpace to matrix functions
fingolfin Nov 21, 2024
fa96542
more
fingolfin Nov 21, 2024
75919c0
more again
fingolfin Nov 21, 2024
b767716
oops, too much copy&paste
fingolfin Nov 22, 2024
f7d1dbf
Ensure r,c non-negative
fingolfin Nov 22, 2024
6392a3c
oops again
fingolfin Nov 22, 2024
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
80 changes: 16 additions & 64 deletions src/Matrix.jl
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -49,11 +49,6 @@ function _check_dim(r::Int, c::Int, a::MatrixElem)
return nothing
end

function _check_bases(a, b)
base_ring(a) == base_ring(b) || throw(DomainError((a, b), "Base rings do not match."))
return nothing
end

_checkbounds(i::Int, j::Int) = 1 <= j <= i

_checkbounds(i::Int, j::AbstractVector{Int}) = all(jj -> 1 <= jj <= i, j)
Expand Down Expand Up @@ -96,55 +91,20 @@ end

function (s::MatSpace{T})(a::MatrixElem{T}) where {T <: NCRingElement}
_check_dim(nrows(s), ncols(s), a)
_check_bases(s, a)
base_ring(s) == base_ring(a) || throw(DomainError((s, a), "Base rings do not match."))
a isa eltype(s) && return a
M = s() # zero matrix
R = base_ring(s)
if R == base_ring(a)
for i = 1:nrows(s), j = 1:ncols(s)
M[i, j] = a[i, j]
end
else
for i = 1:nrows(s), j = 1:ncols(s)
M[i, j] = R(a[i, j])
end
end
return M
return matrix(base_ring(a), b)
fingolfin marked this conversation as resolved.
Show resolved Hide resolved
end

# create a matrix with b on the diagonal
function (s::MatSpace)(b::NCRingElement)
M = s() # zero matrix
R = base_ring(s)
rb = R(b)
for i in 1:min(nrows(s), ncols(s))
M[i, i] = rb
end
return M
return diagonal_matrix(R(b), nrows(s), ncols(s))
end

# convert a Julia matrix
function (a::MatSpace{T})(b::AbstractMatrix{S}) where {T <: NCRingElement, S}
_check_dim(nrows(a), ncols(a), b)
R = base_ring(a)

# minor optimization for MatSpaceElem
if S === T && dense_matrix_type(T) === Generic.MatSpaceElem{T} && all(x -> R === parent(x), b)
return Generic.MatSpaceElem{T}(R, b)
end

# generic code
M = a() # zero matrix
for i = 1:nrows(a), j = 1:ncols(a)
M[i, j] = R(b[i, j])
end
return M
end

# convert a Julia vector
function (a::MatSpace{T})(b::AbstractVector) where T <: NCRingElement
_check_dim(nrows(a), ncols(a), b)
return a(transpose(reshape(b, a.ncols, a.nrows)))
# convert a Julia matrix or vector
function (a::MatSpace{T})(b::AbstractVecOrMat) where T <: NCRingElement
return matrix(base_ring(a), nrows(a), ncols(a), b)
end


Expand Down Expand Up @@ -395,7 +355,7 @@ end
Create an uninitialized matrix over the given ring and dimensions,
with defaults based upon the given source matrix `x`.
"""
similar(x::MatElem, R::NCRing, r::Int, c::Int) = zero_matrix(R, r, c)
similar(x::MatElem, R::NCRing, r::Int, c::Int) = dense_matrix_type(R)(R, undef, r, c)

similar(x::MatElem, R::NCRing) = similar(x, R, nrows(x), ncols(x))

Expand Down Expand Up @@ -879,7 +839,7 @@ function zero!(x::MatrixElem{T}) where T <: NCRingElement
for i = 1:nrows(x), j = 1:ncols(x)
x[i, j] = zero(R)
end
x
return x
end

function add!(c::MatrixElem{T}, a::MatrixElem{T}, b::MatrixElem{T}) where T <: NCRingElement
Expand Down Expand Up @@ -6669,7 +6629,10 @@ function matrix(R::NCRing, arr::MatElem)
end

function matrix(R::NCRing, arr::MatRingElem)
return matrix_space(R, nrows(arr), ncols(arr))(arr)
M = zero_matrix(R, nrows(arr), ncols(arr))
for i in 1:nrows(s), j in 1:ncols(s)
M[i, j] = a[i, j]
end
fingolfin marked this conversation as resolved.
Show resolved Hide resolved
end

function matrix(mat::MatrixElem{T}) where {T<:NCRingElement}
Expand Down Expand Up @@ -6725,16 +6688,7 @@ end

Return the $r \times c$ zero matrix over $R$.
"""
function zero_matrix(R::NCRing, r::Int, c::Int)
arr = Matrix{elem_type(R)}(undef, r, c)
for i in 1:r
for j in 1:c
arr[i, j] = zero(R)
end
end
z = Generic.MatSpaceElem{elem_type(R)}(R, arr)
return z
end
zero_matrix(R::NCRing, r::Int, c::Int) = zero!(dense_matrix_type(R)(R, undef, r, c))
Copy link
Collaborator

Choose a reason for hiding this comment

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

Nice. This will allow some code deduplication in Nemo, where something like this is currently implemented for every Matrix type

Copy link
Member Author

Choose a reason for hiding this comment

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

Well, not quite: there we often will want to omit the zero! because the "undef" matrices are actually already zero initialized. E.g. for ZZMatrix.

So they'd still have to have a method like

zero_matrix(R::ZZRing, r::Int, c::Int) = (dense_matrix_type(R)(R, undef, r, c))

or maybe rather

zero_matrix(R::ZZRing, r::Int, c::Int) = ZZMatrix(r, c)

However this made me realize a problem: we don't require the UndefInitializer constructors to perform a sanity check on r and c (i.e. that they are >= 0). And we probably don't want to (it is not needed when called from similar).

So this method here should actually have such a check. But that's fine. And then we'd keep the existing zero_matrix method for ZZRing as it is. However, we could most or all of these methods from `fmpz_mat.jl:

function (a::ZZMatrixSpace)()
  z = ZZMatrix(nrows(a), ncols(a))
  return z
end

function (a::ZZMatrixSpace)(arr::AbstractMatrix{T}) where {T <: IntegerUnion}
  _check_dim(nrows(a), ncols(a), arr)
  z = ZZMatrix(nrows(a), ncols(a), arr)
  return z
end

function (a::ZZMatrixSpace)(arr::AbstractVector{T}) where {T <: IntegerUnion}
  _check_dim(nrows(a), ncols(a), arr)
  z = ZZMatrix(nrows(a), ncols(a), arr)
  return z
end

function (a::ZZMatrixSpace)(d::ZZRingElem)
  z = ZZMatrix(nrows(a), ncols(a), d)
  return z
end

function (a::ZZMatrixSpace)(d::Integer)
  z = ZZMatrix(nrows(a), ncols(a), flintify(d))
  return z
end

Well maybe the last one should be kept as it is a very minor optimization (as it avoids allocating to "convert" d to a ZZRingElem if it is a bit integer). Though as I said, very few people would go through that interface anyway, so maybe nothing to worry about too much...


zero_matrix(::Type{MatElem}, R::Ring, n::Int, m::Int) = zero_matrix(R, n, m)

Expand All @@ -6750,11 +6704,9 @@ zero_matrix(::Type{MatElem}, R::Ring, n::Int, m::Int) = zero_matrix(R, n, m)
Return the $r \times c$ ones matrix over $R$.
"""
function ones_matrix(R::NCRing, r::Int, c::Int)
z = zero_matrix(R, r, c)
for i in 1:r
for j in 1:c
z[i, j] = one(R)
end
z = dense_matrix_type(R)(R, undef, r, c)
for i in 1:r, j in 1:c
z[i, j] = one(R)
end
return z
end
Expand Down
4 changes: 4 additions & 0 deletions src/generic/Matrix.jl
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -25,6 +25,10 @@ parent(a::MatElem) = matrix_space(base_ring(a), nrows(a), ncols(a))

Return the type of matrices with coefficients of type `T` respectively
`elem_type(S)`.

Implementations of the ring interface only need to provide a method
for the argument a subtype of `NCRingElement`; the other variants are
implemented by calling that method.
"""
dense_matrix_type(::T) where T <: NCRing = dense_matrix_type(elem_type(T))
dense_matrix_type(::T) where T <: NCRingElement = dense_matrix_type(T)
Expand Down
4 changes: 3 additions & 1 deletion test/generic/Matrix-test.jl
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -73,11 +73,13 @@ struct F2Matrix <: AbstractAlgebra.MatElem{F2Elem}
m::Generic.MatSpaceElem{F2Elem}
end

F2Matrix(::F2, ::UndefInitializer, r::Int, c::Int) = F2Matrix(Generic.MatSpaceElem{F2Elem}(F2(), undef, r, c))

AbstractAlgebra.elem_type(::Type{F2MatSpace}) = F2Matrix
AbstractAlgebra.parent_type(::Type{F2Matrix}) = F2MatSpace

AbstractAlgebra.base_ring(::F2MatSpace) = F2()
AbstractAlgebra.dense_matrix_type(::Type{F2}) = F2Matrix
AbstractAlgebra.dense_matrix_type(::Type{F2Elem}) = F2Matrix
AbstractAlgebra.matrix_space(::F2, r::Int, c::Int) = F2MatSpace(F2(), r, c)

AbstractAlgebra.number_of_rows(a::F2Matrix) = nrows(a.m)
Expand Down
Loading