Feedback/minor issues for new `hvncat` n-dimensional concatenation

[BioTurboNick]

Prompted by @matthias314 , I thought it'd be a good idea to have an issue for general feedback and comments on the new hvncat syntax, as people start to use it.

If you find a concrete bug, please open a separate issue and tag me on it.

But for general questions, concerns, documentation suggestions, or minor things, we can collect them here and later turn them into more concrete actions as necessary.

[matthias314]

Here are some comments in addition to #41107. I'm referring to the call syntax

hvncat(dims::Tuple{Vararg{Int}}, row_first, values...)
hvncat(shape::Tuple{Vararg{Tuple}}, row_first, values...)

Whenever I say "dimension", I mean the length of the dims or shape tuple. The implementation currently has dedicated methods for dimensions 0 and 1 and a general method for dimensions at least 2.

Argument checking

The general method seems to do argument checking, but the dedicated ones in dimensions 0 and 1 don't. For example,

hvncat((2,), true, 1, 2, 3)

returns a vector of length 3 instead of producing an error. I think it would make sense if all methods checked their arguments.

0-dimensional case

In dimension 0, hvncat always returns [] (no matter what values is). I would instead expect the following: values must have exactly one element x. If x is an AbstractArray, then hvncat returns some copy of x. (I'm going to discuss the return type below.) Otherwise hvncat returns a 0-dimensional array with unique element x.

This is in analogy with the case where dims is a tuple of length m > 0 containing only 1's. If x is an AbstractArray of dimension n >= m, then the returned value is an Array equal to x (because "excess dimensions" are preserved). If x is not an AbstractArray, then the returned value is an Array of size dims containing the single value x.

Return type

It seems that the general method always returns an Array (as does the method in dimension 0). On the other hand, if a is a BitArray, then vcat(a,a), hcat(a,a) and hvcat(2,a,a,a,a) return BitArray's. Because of the special treatment of the 1-dimensional case, hvncat may also return something different from an Array in dimension 1, which is out of line with the other dimensions. Would it make sense to have a more uniform treatment of all dimensions? Of course, it would be great if hvncat could adapt the return type like vcat and friends, but I don't know how complicated that would be to implement.

[BioTurboNick]

Thanks for the comments, those are good thoughts.

For 0 dimension: Following the example of hvcat((), 1), hvncat((), true, 1) would produce an error. Although the former is a failed bounds check because it is written assuming a non-empty tuple. Producing a 0-d array is interesting. The main intent of these methods is to translate syntax and that isn't representable in the syntax, though that could be added in.

For 1 dimension: Yeah. And really, it should also change whether it dispatches to hcat or vcat depending on row_first, I think.

Return type: yes, I see hvcat uses similar to achieve that. I think that would be easy enough?

[matthias314]

Dimension 0:

Following the example of hvcat((), 1), hvncat((), true, 1)

I wouldn't draw this comparison because I understood from your explanation of the shape form that the first argument of hvcat does not correspond to the first argument (the tuple shape) of hvncat, but to the first element of shape. For example, hvcat((p,q), ...) is equivalent to hvncat(((p,q),(p+q,)), true, ...).

Dimension 1:

it should also change whether it dispatches to hcat or vcat depending on row_first

I see your point. However, then hvncat would not be fully type-stable anymore. (The number of dimensions of the array would depend on the value of row_first.)

Return type: The easier the solution, the better!

[BioTurboNick]

I think you're right, and I think I have a comprehensive set of tests that produces the behavior you suggest. I've also implemented the similar solution in my own branch; I'll have it all in a PR later. If you can take a look below, let me know if you feel one is wrong or I'm missing one. Not shown below are the more rigorous argument checking tests.

As far as type stability goes for the N = 1 shape form, I believe I've structured it to limit the issues there, using Val to produce specializations for each type.

 # 0-dimension behaviors
    @test hvncat(0) == []
    @test hvncat(0, 1) == fill(1)
    @test hvncat(0, [1]) == [1]
    @test_throws ArgumentError hvncat(0, 1, 1)
    @test typed_hvncat(Float64, 0) == Float64[]
    @test typed_hvncat(Float64, 0, 1) == fill(1.0)
    @test typed_hvncat(Float64, 0, [1]) == Float64[1.0]
    @test_throws ArgumentError hvncat(0, 1, 1)
    @test hvncat((), true) == []
    @test hvncat((), true, 1) == fill(1)
    @test hvncat((), true, [1]) == [1]
    @test_throws ArgumentError hvncat((), true, 1, 1)
    @test typed_hvncat(Float64, (), true) == Float64[]
    @test typed_hvncat(Float64, (), true, 1) == fill(1.0)
    @test typed_hvncat(Float64, (), true, [1]) == [1.0]
    @test_throws ArgumentError typed_hvncat(Float64, (), true, 1, 1)

    # 1-dimension behaviors
    # int form
    @test hvncat(1) == []
    @test hvncat(1, 1) == [1]
    @test hvncat(1, [1]) == [1]
    @test hvncat(1, 1, 1) == [1 ; 1]
    @test typed_hvncat(Float64, 1) == Float64[]
    @test typed_hvncat(Float64, 1, 1) == Float64[1.0]
    @test typed_hvncat(Float64, 1, [1]) == Float64[1.0]
    @test typed_hvncat(Float64, 1, 1, 1) == Float64[1.0 ; 1.0]
    # dims form
    @test_throws ArgumentError hvncat((1,), true)
    @test hvncat((2,), true, 1, 1) == [1; 1]
    @test hvncat((2,), true, [1], [1]) == [1; 1]
    @test_throws ArgumentError hvncat((2,), true, 1)
    @test typed_hvncat(Float64, (2,), true, 1, 1) == Float64[1.0; 1.0]
    @test typed_hvncat(Float64, (2,), true, [1], [1]) == Float64[1.0; 1.0]
    @test_throws ArgumentError typed_hvncat(Float64, (2,), true, 1)
    # row_first has no effect with just one dimension of the dims form
    @test hvncat((2,), false, 1, 1) == [1; 1]
    @test typed_hvncat(Float64, (2,), false, 1, 1) == Float64[1.0; 1.0]
    # shape form
    @test hvncat((2,), true, 1, 1) == [1 1]
    @test hvncat((2,), true, [1], [1]) == [1 1]
    @test_throws ArgumentError hvncat((2,), true, 1)
    @test hvncat((2,), false, 1, 1) == [1; 1]
    @test hvncat((2,), false, [1], [1]) == [1; 1]
    @test typed_hvncat(Float64, (2,), true, 1, 1) == Float64[1.0 1.0]
    @test typed_hvncat(Float64, (2,), true, [1], [1]) == Float64[1.0 1.0]
    @test_throws ArgumentError typed_hvncat(Float64, (2,), true, 1)
    @test typed_hvncat(Float64, (2,), false, 1, 1) == Float64[1.0; 1.0]
    @test typed_hvncat(Float64, (2,), false, [1], [1]) == Float64[1.0; 1.0]

[matthias314]

I'll have a look at it. Something else: I've looked at the source code for hvncat (in master). I think that several Vararg types can be written in a shorter form:

Tuple{Vararg{Any, 1}} == Tuple{Any}
Tuple{Vararg{Int, N}} == NTuple{N, Int}
Tuple{Vararg{Tuple, N}} == NTuple{N, Tuple}
Tuple{Vararg} == Tuple

[matthias314]

Regarding the tests: I think that the tests listed under "shape form" again test the dims form. Partially related to this, the following tests appear twice:

@test hvncat((2,), false, 1, 1) == [1; 1]
@test_throws ArgumentError hvncat(0, 1, 1)
@test_throws ArgumentError hvncat((2,), true, 1)
@test_throws ArgumentError typed_hvncat(Float64, (2,), true, 1)
@test typed_hvncat(Float64, (2,), false, 1, 1) == Float64[1.0; 1.0]

[BioTurboNick]

I suppose I should not have whipped it up late at night 😂

With respect to types, it has to be Tuple and not NTuple because the former is more general. And I'll try the first and last one but I think Julia got confused when I didn't keep puting Vararg consistently.

[matthias314]

it has to be Tuple and not NTuple because the former is more general.

This is not really important, but are you sure? If you assign some positive integer value to N, then all the == formulas I listed above evaluate to true in Julia.

[BioTurboNick]

it has to be Tuple and not NTuple because the former is more general.

This is not really important, but are you sure? If you assign some positive integer value to N, then all the == formulas I listed above evaluate to true in Julia.

Huh... I must be misremembering something I tried out earlier. Thanks!

[BioTurboNick]

Note to self: This should be closed when 1.7 is released.