Remove full from similar(full(X), T, dims) calls in generic concatenation methods

[Sacha0]

Towards reintroducing #17079, this pull request removes full from similar(full(X), T, dims) calls in the generic concatenation methods.

For an explanation of those full calls' purpose, see the first paragraph of #17079 (comment). See the preceding comments in that thread to understand why displacing those full calls is necessary to reintroduce #17079 .

Contrary to the expectation I expressed in that comment, removing those full calls appears to improve the degree to which the generic concatenation methods preserve structure:

The code in this gist determines, for all pairwise combinations of dense vectors, dense matrices, sparse vectors, sparse matrices, special matrices, and triangular, symmetric, and hermitian annotations of both dense and sparse matrices, the return type of prototypical vcat, hcat, hvcat, and cat calls, and prints the mappings catmeth(typeA, typeB) -> typeRet. This gist provides the output of that code on master, and this gist the same with this PR. This gist provides the diff. (Update: These gists are up to date, so the diff is now empty.)

tl;dr of details To sum up the diff: When a sparse matrix or vector appears as the first argument in a concatenation call for which there exists no specialized method, on master the result is an Array, whereas with this PR the result remains sparse. Otherwise the results are identical.

This change makes #2326 more apparent in that, for example, hcat(SparseMatrixCSC, Bidiagonal) would now produce a sparse matrix whereas hcat(Bidiagonal, SparseMatrixCSC) would still produce a dense matrix. Modifying the Unions in this and the accompanying method definitions to include special matrix types would resolve that issue for special matrix types. But the same cannot be done for the annotation types. And absent the ability to write methods with Varargs in positions other than last, I do not immediately see a way to resolve that issue for annotation types. (Edit: Ref. #13130. Combinatorial explosion precludes extending the present strategy much?) Making the change necessary to handle special matrix types while punting to #2326 on annotation types seems attractive. Other solutions? Thanks!

[tkelman]

x-ref the related #15172 and #16722

[martinholters]

I'd also like to point at #16740 (once I get to working on that again), see especially #16740 (comment), where hcat (and friends) basically just decide the desired return type and then forward to typed_hcat (or similar). Hopefully that simplifies writing specializations to pick the correct return type, and if that still fails, allows the caller to override it as needed.

[Sacha0]

AV i686 failure looks like #17662?

[Sacha0]

I've updated the gists following JuliaLang/LinearAlgebra.jl#349 and #17680. Only one set of discrepancies remain: When the first argument is a SparseMatrixCSC or SparseVector and the second argument is an annotation type (<:AbstractTriangular, Symmetric, or Hermitian), on master the result is dense whereas with this PR the result is sparse. JuliaLang/LinearAlgebra.jl#350 causes this.

[Sacha0]

Gists updated following JuliaLang/LinearAlgebra.jl#350. The diff is now empty, so this PR and subsequent reinstatement of #17079 should be viable now. (I will cherry pick #17079 back together if / when this clears, unless there is a better approach to reinstating #17079?). Best!

[tkelman]

let's keep it busy @nanosoldier runbenchmarks(ALL, vs = ":master")

[nanosoldier]

Your benchmark job has completed - no performance regressions were detected. A full report can be found here. cc @jrevels

[Sacha0]

Nanosoldier seems pleased. All well? Thanks!

[Sacha0]

Absent objections or requests for time, I plan to merge this Monday (PDT). Best!

[tkelman]

Is this going to break DataArrays again or has it been refactored to avoid relying on this?

[Sacha0]

Removing these full(X) calls should not break DataArrays; removal (rather than replacement with convert(Array, X)) was the (hypothetical) fix for JuliaStats/DataArrays.jl#208. But testing DataArrays on this branch prior to merge would be wise. @martinholters, do you have min repro code for JuliaStats/DataArrays.jl#208 that I could test? Thanks!

[martinholters]

Too bad Pkg.test("DataArrays") fails on current master. I guess

using DataArrays
T = Float64
n = 1000
a = Array(T, n)
na = bitrand(n)
nna = sum(na)
da = DataArray(a, na)
isequal(sort(da), [DataArray(sort(dropna(da))); DataArray(T, nna)])

should be the relevant part of the test that failed last time.

[Sacha0]

Thanks @martinholters! Results on this branch are identical to those on master. Good to merge, @tkelman @martinholters? Best!

[Sacha0]

Absent objections or requests for time, I plan to merge this Friday (PDT). Best!

[martinholters]

No objections from my side.