Change similar, zero_matrix, ones_matrix to use UndefInitializer constructors

[fingolfin]

This is incompatible with older Nemo versions which did not provide UndefInitializer constructors for their matrices. Thus I've added the "breaking" label.

Note quite done yet, I'd like to convert more places to use the undef constructors. Should be OK to review now.

[lgoettgens]

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

[fingolfin]

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

[codecov]

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 88.21%. Comparing base (429961e) to head (6392a3c).
Report is 15 commits behind head on master.

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #1909      +/-   ##
==========================================
+ Coverage   88.17%   88.21%   +0.04%     
==========================================
  Files         120      120              
  Lines       30298    30945     +647     
==========================================
+ Hits        26715    27298     +583     
- Misses       3583     3647      +64     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[fingolfin]

There is still some unfortunate code duplication left, which currently forces matrix implementations to produce more code than idea. For example, there are two ways to construct a scalar matrix with a value x on the diagonal:

1. via diagonal_matrix(ringElem, n)
2. via matrix_space(parent(ringElem), n, n)(ringElem)

Currently the default implementations for these and similar methods are mostly independent, meaning matrix types need to overload both if they have optimized versions.

In contrast, matrix_space(R, n, n)() is implemented by delegating to zero_matrix(R, n, n)

So we could follow that model and delegate this way.

However, note that matrix_space methods don't need to validate the rows/cols inputs. So for example for ZZMatrix we have:

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

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

versus

function zero_matrix(R::ZZRing, r::Int, c::Int)
  if r < 0 || c < 0
    error("dimensions must not be negative")
  end
  z = ZZMatrix(r, c)
  return z
end

# no custom diagonal_mat method in Nemo, but it might look like this:
function diagonal_matrix(d::ZZRingElem, r::Int, c::Int)
  if r < 0 || c < 0
    error("dimensions must not be negative")
  end
  z = ZZMatrix(r, c, d)
  return z
end

But overall I say we should not worry about this very minor inefficiency because, (a) it is negligible compared to the actual cost for allocating a matrix, and (b) hardly anyone will go through the matrix space API anyway.

Here are some measurements to support my performance claim:

julia> @benchmark ZZMatrix(3, 3)
BenchmarkTools.Trial: 10000 samples with 991 evaluations.
 Range (min … max):   46.544 ns … 94.404 μs  ┊ GC (min … max):  0.00% … 28.71%
 Time  (median):      55.373 ns              ┊ GC (median):     0.00%
 Time  (mean ± σ):   139.305 ns ±  2.619 μs  ┊ GC (mean ± σ):  16.79% ±  0.92%

         ▁▂▄▆▆█▇█▇▇▇▆▅▅▄▃▂▁
  ▁▂▃▄▅▆▇██████████████████▇▆▅▄▃▂▂▂▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁ ▄
  46.5 ns         Histogram: frequency by time         79.5 ns <

 Memory estimate: 176 bytes, allocs estimate: 3.

julia> @benchmark zero_matrix($ZZ, 3, 3)
BenchmarkTools.Trial: 10000 samples with 991 evaluations.
 Range (min … max):   46.249 ns … 100.167 μs  ┊ GC (min … max):  0.00% … 28.82%
 Time  (median):      55.121 ns               ┊ GC (median):     0.00%
 Time  (mean ± σ):   134.377 ns ±   2.624 μs  ┊ GC (mean ± σ):  16.74% ±  0.89%

            ▃▂▄▅▄▅▆▆▆▇▆▆▇▇▇█▆▄▃▃▁▁
  ▁▂▂▃▄▅▆▆█▇██████████████████████▇▇▅▄▄▃▃▂▂▂▂▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁ ▄
  46.2 ns          Histogram: frequency by time         71.9 ns <

 Memory estimate: 176 bytes, allocs estimate: 3.

Here the mean time without the check even seems to 5 nanoseconds higher than that with the check. Of course that's just a fluke and if I repeat this several times it shifts around which one looks "faster". But that's kinda my point (and yeah of course I tested on my work laptop which means a bunch of stuff is running in the background; serious benchmarking needs a dedicated benchmark machine. But that seems irrelevant).

Based on this I've now changed (s::MatSpace)(b::NCRingElement) to delegate to diagonal_matrix, and added a few other delegations. See second commit in this PR.

[fingolfin]

The NemoCI failures really confused me until I realized they are still with 0.47.3 not 0.47.4 -- turns out while Nemocas/Nemo.jl#1945 was merged last week, which marked Nemo 0.47.4, we never registered it. Oops! So I just triggered that and then the tests can re-run afterwards (and hopefully pass).

Of course this is still breaking for Nemo <= 0.47.3, but I'd like to really be sure it works with newer versions (seems to be the case when I run the tests locally)