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

Remove the internal helper `_similar` which untangles the `MatElem` and
`MatRingElem` code further and makes it easier to see what's going on.

Also change `similar` for `MatElem` to default to calling `zero_matrix`.
With this `similar(::ZZMatrix)` in Nemo returns a new `ZZMatrix` instead
of a `AbstractAlgebra.Generic.MatSpaceElem{ZZRingElem}`.

Also untangle `zero` and `similar` methods for `MatElem` versus `MatRingElem`.

Author: fingolfin

docs/src/matrix.md

@@ -433,7 +433,7 @@ diagonal_matrix(::RingElement, ::Int, ::Int)
 
 ```@docs
 zero(::MatSpace)
-zero(::MatrixElem{T}, ::Ring) where T <: RingElement
+zero(::MatElem{T}, ::Ring) where T <: RingElement
 ```
 
 ```@docs

src/MatRing.jl

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

src/Matrix.jl

@@ -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)
 
@@ -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)
+  
+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
@@ -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$.
 """

src/generic/Matrix.jl

@@ -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
+
 function copy(d::MatSpaceElem{T}) where T <: NCRingElement
    return MatSpaceElem{T}(base_ring(d), copy(d.entries))
 end

test/Rings-conformance-tests.jl

@@ -740,6 +740,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
@@ -752,6 +756,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
@@ -767,12 +794,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

test/generic/Matrix-test.jl

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