Remove duplicate *cat implementations

[lgoettgens]

Resolves #1466.

The *cat(::MatElem...) are bad duplicates to methods in src/Matrix.jl.
The *cat(::Vector{T}) where T < MatElem shouldn't have been there in the first place. They originate form Nemo/Hecke. They shadow the *cat(::Vector...) dispatch from base with a 1-elem vararg, and something similar isn't available anywhere else either. So I suggest to remove them. Let's talk about it this afternoon.

[codecov]

Codecov Report

Merging #1467 (5a51616) into master (56c8e00) will increase coverage by 0.91%.
Report is 20 commits behind head on master.
The diff coverage is 50.00%.

@@            Coverage Diff             @@
##           master    #1467      +/-   ##
==========================================
+ Coverage   84.72%   85.63%   +0.91%     
==========================================
  Files         110      111       +1     
  Lines       29365    31223    +1858     
==========================================
+ Hits        24880    26739    +1859     
+ Misses       4485     4484       -1     

Files	Coverage Δ
src/NemoStuff.jl	2.39% <ø> (+1.17%)	⬆️
src/Matrix.jl	92.44% <50.00%> (+0.70%)	⬆️

... and 23 files with indirect coverage changes

📣 Codecov offers a browser extension for seamless coverage viewing on GitHub. Try it in Chrome or Firefox today!

[thofma]

If we remove them, we need an equally fast alternative for concatenating a list of matrices.

[lgoettgens]

I think you don't like vcat(vec...) due to compilation overhead, right?

But there is already _vcat and _hcat that each take a Vector of matrices. They are part of the internals of our *cat implementation in src/Matrix.jl. I think these are exactly what you want. We can just export them or find a better name.

[thofma]

This is exactly the point. One should never splat a vector. Base says to use reduce(vcat, Vector{<:Matrix}), because there is a special method for that. Ideally we have the same, as we always try to do. It is also advertised in the vcat docstring.

[lgoettgens]

This is exactly the point. One should never splat a vector. Base says to use reduce(vcat, Vector{<:Matrix}), because there is a special method for that. Ideally we have the same, as we always try to do. It is also advertised in the vcat docstring.

_vcat and _hcat are used for a reduce overload as well.

[thofma]

Yes, they should be removed too:

julia> reduce(vcat, Union{}[])
ERROR: MethodError: reduce(::typeof(vcat), ::Vector{Union{}}) is ambiguous.

Candidates:
  reduce(::typeof(vcat), A::AbstractVector{<:AbstractVecOrMat})
    @ Base abstractarray.jl:1705
  reduce(::typeof(vcat), A::AbstractVector{<:MatrixElem})
    @ AbstractAlgebra ~/.julia/packages/AbstractAlgebra/7siWW/src/Matrix.jl:6499

Possible fix, define
  reduce(::typeof(vcat), ::AbstractVector{Union{}})

Stacktrace:
 [1] top-level scope
   @ REPL[90]:1

[thofma]

Maybe to summarise my point. All these methods need to go. But I don't want to do it unless we have a good substitute, which is not a new function with some obscure name differing from Base.

[fingolfin]

Do we really need to remove this? I just tried with removing just the preceding methods, and vcat(Union{}[]) as well as vcat() worked just fine.

Same question for the other methods taking ::MatElem... args

[lgoettgens]

They already exist in better in src/Matrix.jl. So these removed methods here get never actually called

[thofma]

We plan to go forward with this PR here, but I think we decided to first fix all the call sites to make the transition a bit smoother.

[lgoettgens]

thofma/Hecke.jl#1253 will fix many obvious call sites in Hecke.

[fingolfin]

Is this ready for merging into a breaking AA release? Or anything that should be done before (then perhaps convert it to draft)?

[lgoettgens]

From my POV this can be merged. @thofma do you have any objections?