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

Simplify and fix similar and zero for our matrix types #1890

Merged
merged 9 commits into from
Nov 12, 2024
Merged

Simplify and fix similar and zero for our matrix types #1890

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
9 commits
Select commit Hold shift + click to select a range
addab0c
Simplify and fix `similar` for our matrix types
fingolfin Oct 31, 2024
edf1130
untangle
fingolfin Nov 8, 2024
8d64442
preserve Generic.MatSpaceElem
fingolfin Nov 8, 2024
bcdee6b
Test more variants of 'similar'
fingolfin Nov 8, 2024
23d055c
Fix zero(MatrixElem) to call suitable similar methods
fingolfin Nov 8, 2024
f7da828
Remove redundant similar method
fingolfin Nov 8, 2024
cea07e2
Untangle docstrings for similar, zero; MatElem, MatRingElem
fingolfin Nov 12, 2024
71d6f09
Restore similar/zero method for MatRingElem with row *and* column arg
fingolfin Nov 12, 2024
fd156e4
fix
fingolfin Nov 12, 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
2 changes: 1 addition & 1 deletion docs/src/matrix.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -435,7 +435,7 @@ diagonal_matrix(::RingElement, ::Int, ::Int)

```@docs
zero(::MatSpace)
zero(::MatrixElem{T}, ::Ring) where T <: RingElement
zero(::MatElem{T}, ::Ring) where T <: RingElement
```

```@docs
Expand Down
33 changes: 19 additions & 14 deletions src/MatRing.jl
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -79,21 +79,23 @@ is_finite(R::MatRing) = iszero(nrows(a)) || is_finite(base_ring(R))
###############################################################################

@doc raw"""
similar(x::Generic.MatrixElem, R::NCRing=base_ring(x))
similar(x::Generic.MatrixElem, R::NCRing, r::Int, c::Int)
similar(x::Generic.MatrixElem, r::Int, c::Int)
similar(x::MatRingElem, R::NCRing, n::Int)
similar(x::MatRingElem, R::NCRing)
similar(x::MatRingElem, n::Int)
similar(x::MatRingElem)

Create an uninitialized matrix over the given ring and dimensions,
with defaults based upon the given source matrix `x`.
Create an uninitialized matrix ring element over the given ring and dimension,
with defaults based upon the given source matrix ring element `x`.
"""
similar(x::MatRingElem, R::NCRing, n::Int) = _similar(x, R, n, n)

similar(x::MatRingElem, R::NCRing=base_ring(x)) = similar(x, R, degree(x))
function similar(x::MatRingElem, R::NCRing=base_ring(x), n::Int=degree(x))
TT = elem_type(R)
M = Matrix{TT}(undef, (n, n))
return Generic.MatRingElem{TT}(R, M)
end

similar(x::MatRingElem, n::Int) = similar(x, base_ring(x), n)

# TODO: deprecate these:
function similar(x::MatRingElem{T}, R::NCRing, m::Int, n::Int) where T <: NCRingElement
m != n && error("Dimensions don't match in similar")
return similar(x, R, n)
Expand All @@ -102,18 +104,21 @@ end
similar(x::MatRingElem, m::Int, n::Int) = similar(x, base_ring(x), m, n)

@doc raw"""
zero(x::MatrixElem, R::NCRing=base_ring(x))
zero(x::MatrixElem, R::NCRing, r::Int, c::Int)
zero(x::MatrixElem, r::Int, c::Int)
zero(x::MatRingElem, R::NCRing, n::Int)
zero(x::MatRingElem, R::NCRing)
zero(x::MatRingElem, n::Int)
zero(x::MatRingElem)

Create a zero matrix over the given ring and dimensions,
with defaults based upon the given source matrix `x`.
Create a zero matrix ring element over the given ring and dimension,
with defaults based upon the given source matrix ring element `x`.
"""
zero(x::MatRingElem, R::NCRing, n::Int) = zero!(similar(x, R, n))
zero(x::MatRingElem, R::NCRing=base_ring(x), n::Int=degree(x)) = zero!(similar(x, R, n))
zero(x::MatRingElem, n::Int) = zero!(similar(x, n))

# TODO: deprecate these
zero(x::MatRingElem, R::NCRing, r::Int, c::Int) = zero!(similar(x, R, r, c))
zero(x::MatRingElem, r::Int, c::Int) = zero!(similar(x, r, c))

################################################################################
#
# Copy and deepcopy
Expand Down
50 changes: 28 additions & 22 deletions src/Matrix.jl
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -229,18 +229,6 @@ Return the zero matrix in the given matrix space.
"""
zero(a::MatSpace) = a()

@doc raw"""
zero(x::MatrixElem{T}, R::NCRing, r::Int, c::Int) where T <: NCRingElement
zero(x::MatrixElem{T}, R::NCRing=base_ring(x)) where T <: NCRingElement
zero(x::MatrixElem{T}, r::Int, c::Int) where T <: NCRingElement

Return a zero matrix similar to the given matrix, with optionally different
base ring or dimensions.
"""
zero(x::MatrixElem{T}, R::NCRing=base_ring(x)) where T <: NCRingElement = zero(x, R, nrows(x), ncols(x))
zero(x::MatrixElem{T}, R::NCRing, r::Int, c::Int) where T <: NCRingElement = zero!(similar(x, R, r, c))
zero(x::MatrixElem{T}, r::Int, c::Int) where T <: NCRingElement = zero(x, base_ring(x), r, c)

@doc raw"""
one(a::MatSpace)

Expand Down Expand Up @@ -398,19 +386,37 @@ end
#
###############################################################################

function _similar(x::MatrixElem{T}, R::NCRing, r::Int, c::Int) where T <: NCRingElement
TT = elem_type(R)
M = Matrix{TT}(undef, (r, c))
z = x isa MatElem ? Generic.MatSpaceElem{TT}(R, M) : Generic.MatRingElem{TT}(R, M)
return z
end

similar(x::MatElem, R::NCRing, r::Int, c::Int) = _similar(x, R, r, c)
@doc raw"""
similar(x::MatElem{T}, R::NCRing, r::Int, c::Int) where T <: NCRingElement
similar(x::MatElem{T}, R::NCRing) where T <: NCRingElement
similar(x::MatElem{T}, r::Int, c::Int) where T <: NCRingElement
similar(x::MatElem{T}) where T <: NCRingElement

similar(x::MatElem, R::NCRing=base_ring(x)) = similar(x, R, nrows(x), ncols(x))
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)
Copy link
Collaborator

Choose a reason for hiding this comment

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

This could be changed in the future to use the UndefInitializer, right? (but not needed right now)
so something like dense_matrix_type(R)(R, undef, r, c)

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes, that's the plan (but we can't change it now because that'd be a breaking change and wouldn't work with older Nemo versions.


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

similar(x::MatElem, r::Int, c::Int) = similar(x, base_ring(x), r, c)

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

@doc raw"""
zero(x::MatElem{T}, R::NCRing, r::Int, c::Int) where T <: NCRingElement
zero(x::MatElem{T}, r::Int, c::Int) where T <: NCRingElement
zero(x::MatElem{T}, R::NCRing) where T <: NCRingElement
zero(x::MatElem{T}) where T <: NCRingElement

Create an zero matrix over the given ring and dimensions,
with defaults based upon the given source matrix `x`.
"""
zero(x::MatElem{T}, R::NCRing) where T <: NCRingElement = zero(x, R, nrows(x), ncols(x))
zero(x::MatElem{T}) where T <: NCRingElement = zero(x, nrows(x), ncols(x))
zero(x::MatElem{T}, R::NCRing, r::Int, c::Int) where T <: NCRingElement = zero!(similar(x, R, r, c))
zero(x::MatElem{T}, r::Int, c::Int) where T <: NCRingElement = zero!(similar(x, r, c))

###############################################################################
#
# Canonicalisation
Expand Down Expand Up @@ -1014,7 +1020,7 @@ function +(x::T, y::MatrixElem{T}) where {T <: NCRingElem}
end

@doc raw"""
+(x::Generic.MatrixElem{T}, y::T) where {T <: RingElem}
+(x::MatrixElem{T}, y::T) where {T <: RingElem}

Return $x + S(y)$ where $S$ is the parent of $x$.
"""
Expand Down
5 changes: 5 additions & 0 deletions src/generic/Matrix.jl
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -58,6 +58,11 @@ Base.isassigned(a::Union{Mat,MatRingElem}, i, j) = isassigned(a.entries, i, j)
#
################################################################################

function similar(x::MatSpaceElem{T}, r::Int, c::Int) where T <: NCRingElement
M = Matrix{T}(undef, (r, c))
return MatSpaceElem{T}(base_ring(x), M)
end
Comment on lines +61 to +64
Copy link
Member Author

Choose a reason for hiding this comment

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

This method would go if we agree on the proposal to forbid MatSpaceElem{T} instances if dense_matrix_type(T) != MatSpaceElem{T}.

But in the meantime it is necessary to preserve Nemo tests.


function copy(d::MatSpaceElem{T}) where T <: NCRingElement
z = similar(d)
for i = 1:nrows(d)
Expand Down
37 changes: 36 additions & 1 deletion test/Rings-conformance-tests.jl
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -703,6 +703,10 @@ function test_MatSpace_interface(S::MatSpace; reps = 20)
R = base_ring(S)
T = elem_type(R)

@test base_ring_type(S) == typeof(R)
@test parent_type(ST) == typeof(S)
@test dense_matrix_type(R) == ST

@testset "MatSpace interface for $(S) of type $(typeof(S))" begin

@testset "Constructors" begin
Expand All @@ -715,6 +719,29 @@ function test_MatSpace_interface(S::MatSpace; reps = 20)
@test a == matrix(R, T[a[i, j] for i in 1:nrows(a), j in 1:ncols(a)])
@test a == matrix(R, nrows(S), ncols(S),
T[a[i, j] for i in 1:nrows(a) for j in 1:ncols(a)])

b = similar(a)
@test b isa ST
@test nrows(b) == nrows(S)
@test ncols(b) == ncols(S)

b = similar(a, nrows(S)+1, ncols(S)+1)
@test b isa ST
@test nrows(b) == nrows(S)+1
@test ncols(b) == ncols(S)+1

b = similar(a, R)
@test b isa MatElem
#@test b isa ST # undefined
@test nrows(b) == nrows(S)
@test ncols(b) == ncols(S)

b = similar(a, R, nrows(S)+1, ncols(S)+1)
@test b isa MatElem
#@test b isa ST # undefined
@test nrows(b) == nrows(S)+1
@test ncols(b) == ncols(S)+1

end
@test iszero(zero_matrix(R, nrows(S), ncols(S)))
end
Expand All @@ -730,12 +757,20 @@ function test_MatSpace_interface(S::MatSpace; reps = 20)
for k in 1:reps
a = test_elem(S)::ST
A = deepcopy(a)
@test A isa ST

b = zero_matrix(R, nrows(a), ncols(a))
@test b isa ST
for i in 1:nrows(a), j in 1:ncols(a)
b[i, j] = a[i, j]
end
@test b == a
@test transpose(transpose(a)) == a

t = transpose(a)
@test t isa ST
@test nrows(t) == ncols(S)
@test ncols(t) == nrows(S)
@test transpose(t) == a
@test a == A
end
end
Expand Down
1 change: 0 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 @@ -86,7 +86,6 @@ AbstractAlgebra.base_ring(::F2Matrix) = F2()

Base.getindex(a::F2Matrix, r::Int64, c::Int64) = a.m[r, c]
Base.setindex!(a::F2Matrix, x::F2Elem, r::Int64, c::Int64) = a.m[r, c] = x
Base.similar(x::F2Matrix, R::F2, r::Int, c::Int) = F2Matrix(similar(x.m, r, c))

function AbstractAlgebra.zero_matrix(R::F2, r::Int, c::Int)
mat = Matrix{F2Elem}(undef, r, c)
Expand Down
Loading