remove vcat(A::Vector{<: MatrixElem})

[rfourquet]

This is commiting "type-piracy".
This method is already defined in Base and gives a different result,
i.e. vcat on a Vector returns another Vector equal to the input.
Similar for hcat.

[thofma]

MatrixElem is not defined in Base. Why is this type piracy?

[rfourquet]

Ok, this is type piracy for a loose definition of "type piracy" (defining vcat on a subset of Vectors where both vcat and Vector belong to Base.)
But the point is that this method does something totally unexpected by diverging from vcat's definition in Base, without any clear benefit.

[thofma]

The benefit is that I don't have to splat the arguments when I want to concatenate a vector of matrices. z = zero_matrix(ZZ, 1, 1); y = vcat([z, z, z, z]). Isn't this a benefit? I give him a collection of things to concatenate.

Also splatting is hard on the compiler and pretty much non-discoverable for a user.

[thofma]

Or what if I have an iterator it of matrices. Should vcat(zero_matrix(ZZ, 1, 1) for i in 1:10) also be disallowed? Do I have to materialize it first?

[rfourquet]

vcat([z, z, z, z]

I think the performance of this will be sensibly equal to that of a literal vcat(z, z, z, z). That said, if Z = [z, z, z, z], then vcat(Z...) seems to be significantly slower. I guess vcat is designed for cases when you don't need to splat an existing collection with ....

Also splatting is [...] pretty much non-discoverable for a user.

I argue that the vcat(A::Vector{<: MatrixElem}) method is bad for a user coming from Julia (who already know vcat), in how it hijacks the meaning of Base's vcat in the name of convenience. The less corner cases, the better. One example coming to my mind is [1:9] which in the past would create a vector with 9 entries, 1 to 9 (it would be equivalent to collect(1:9), presumably for convenience. But what if I want a vector with a single element, 1:9 itself? So the special case was removed and [1:9] now creates a one-element vector.

Should vcat(zero_matrix(ZZ, 1, 1) for i in 1:10) also be disallowed?

This example actually supports my point. The existing vcat method (in AA) handles only Vector, not collections/generators. There is no way to handle all kind of collection. So at one point, you need another function name, e.g. vcat_collection. One function (vcat) takes each element as separate arguments, another (vcat_collection) takes as argument one collection. This is similar to max vs maximum in Base. People tried seriously to unify these two function into only one name, but it didn't work.

[thofma]

As you noticed yourself, it is not only convenience, but also a matter of speed. Splatting is not for free.

Base is only running into trouble and ambiguities since they want to treat numbers as arrays/collections. We don't do this for ring elements so there are no corner cases.

The julia concatenation functions are already full of corner cases:

julia> [1 2; 3 4]
2×2 Array{Int64,2}:
 1  2
 3  4

julia> A = [1 2; 3 4]
2×2 Array{Int64,2}:
 1  2
 3  4

julia> [A A; A A] # Why not Arry{Array{Int64, 2}, 2} or Array{Int64, 4}?
4×4 Array{Int64,2}:
 1  2  1  2
 3  4  3  4
 1  2  1  2
 3  4  3  4

[wbhart]

I agree with @thofma . This is definitely not type piracy. We have checked carefully with the Julia people that it is only type piracy if it overloads a Base function for a Base type. Otherwise, how would we define exp of a power series for example? The function exp is a Base function, but one has to overload it for one's own types.

I also agree that in some cases the Julia conventions are not workable for us. So I have no problem with doing something different.

[rfourquet]

I agree that it can be a matter of speed, but it doesn't have to be the same name. If splatting becomes necessary with vcat, I agree that a non-splatting version should be defined (_vcat currently, or an hypothetical vcat_collection), and vcat be defined in terms of it.

AFAICT (I may very well be wrong), with the "nice" syntax (i.e. literal [A B] / [A; B]) of vcat / hcat, you can't use splatting. So to use the vcat(A::Vector{<: MatrixElem}) method (where A is not a literal vector) you have to use the function call anyway, then why not use a different name which doesn't conflict with the current meaning of hcat?

I have nothing against having useful functions, but am very opposed to give a different meaning to a Base function in AbstractAlgebra, which is bound to create confusion for users. In this case, it costs so little to create a new function instead of re-using an existing one that it's not worth it (changing the meaning of vcat) in my opinion.

[rfourquet]

I take back "type piracy". Real type piracy is even necessary sometimes, so it's not bad in itself, and this is not my actual point here.

in some cases the Julia conventions are not workable for us

Sure, but in this case there is nothing special about AbstractAlgebra compared to Base. Needing to vcat a number of arrays stored in a collection A is unlikely to be rarer than for a collection of AA matrices stored in A. And renaming vcat(A::Vector{<: MatrixElem}) to vcat_collection(A) (or any other name) has nothing unworkable, and has the added benefit to allow passing any kind of iterable, instead of just Vector.

The julia concatenation functions are already full of corner cases

I don't follow. If I understand correctly, vcat/hcat discriminates between arrays and non-arrays. Non-arrays are somehow treated as 0-dim arrays. At most this can be seen as a convenience for entering literal arrays (i.e. to be able to type [1 2; 3 4] instead of [fill(1) fill(2); fill(3) fill(4)]), but there is no contradiction. Unlike vcat(A::Vector{<: MatrixElem}) which means that vcat(::Vector) sometimes will return a vector, unless the eltype of the vector is MatrixElem, in which case it will call vcat on its elements. This is gratuitous confusion.

[wbhart]

Ok, I didn't quite understand that the issue is related only to something added yesterday. So I have a tendency to slightly agree with @rfourquet that this can be removed, at least until it becomes absolutely necessary.

But it might be tied up with some other issues @thofma is trying to resolve, so I'll wait until we have a clear concensus before merging this.