RFC: Specialize promotion for concatenations

[pabloferz]

Currently for concatenation purposes, we need promotion mechanism that takes eltypes of AbstractArrays and typeof of anything else and promotes these (see #19387 (comment)).

There are two proposals that occur to me to handle this:

1. Define a eltypeof (or eltype_or_typeof) method that gives the appropriate type depending on the argument and pass that to promote_eltype (this PR)
2. Or define a new promote_eltypeof that handles properly mixtures of AbstractArrays and scalars leaving promote_eltype alone (this PR).

The first approach shouldn't change the behavior of promote_eltype when called over AbstractArrays only, that's why I'm inclined to go that route but a proper interface would have to be decided given how things like strings are both scalar and collection under different circumstances. Anyway, it'd be useful to see what other people think.

[nalimilan]

I don't think AbstractArray is the correct interface for this, cf. the discussion we've had at #17053. Strings are always the annoying case which is half-collection and half-scalar...

[pabloferz]

@nalimilan You're right. For now I guess I could go with the second approach for the sake of having concatenations working properly and leave the discussion of a general interface for later.

[andreasnoack]

I think we should avoid adding another ad hoc promote function. We already have

julia> filter(t -> ismatch(r"promote", string(t)) && ismatch(r"^((?!\#).)*$", string(t)), names(Base, true))
15-element Array{Symbol,1}:
 :promote
 :promote_array_type
 :promote_eltype
 :promote_eltype_op
 :promote_op
 :promote_result
 :promote_rule
 :promote_shape
 :promote_to_supertype
 :promote_type
 :promote_typeof
 :promote_union
 :r_promote
 :r_promote_type
 :rcum_promote_type

If this only fixes [["a"], "b"] then I'm not sure it is worth the clutter since Vector{Any} is not wrong, you could do [["a"], ["b"]] if you really cared and you maybe you should just push!(["a"], "b") anyway. Do we know of more cases where this would help?

[pabloferz]

@andreasnoack I was hesitant about this given all the *promote* methods as you pointed out, but this not only fixes the above case (I should add more non-trivial tests).

This should help with part of #19387 (comment).

[pabloferz]

I'd like to unify some of the *promote* methods, e.g. promote_op and promote_eltype_op or promote_typeof, promote_eltype and promote_eltypeof (from this PR), but thats a harder task and requires careful considerations that might fit better in another PR.

[andreasnoack]

I'd like to unify some of the promote methods, e.g. promote_op and promote_eltype_op or promote_typeof, promote_eltype and promote_eltypeof (from this PR), but thats a harder task and requires careful considerations that might fit better in another PR.

That would be great and I agree that it belongs in another PR.

[nalimilan]

Even if the breakage is in my package, I'm not sure we need to restore the previous behavior for vcat(["a"], "b"). What matters is that we choose a consistent design for all collections.

But the new/current behavior does not sound correct either, since vcat(["a"], "b") returns a Vector{Any} which only contains String entries. If we consider String as a collection, it should return Any["a", 'b']. If we consider it as a scalar, it should return String["a", "b"]. The latter behavior is probably the most useful one, but the former would be the most consistent.

Anyway I now see that there already are special cases for AbstractArray, in particular in catsize, so it's probably OK to merge this for now, and look for a more comprehensive fix later.

[pabloferz]

@nalimilan My perception is that the *cat methods should treat as scalars all types without a shape, as it needs a shape to decide if the dimensions of the objects to concatenate are compatible.

Additionally, here is an example that fails with the new behavior, that should work, and that this PR fixes

vcat((1,), (2.0,))
# currently fails trying to construct a Vector{Float64}
# but after this PR returns
Tuple{Real}[(1,), (2.0,)]

[nalimilan]

That's fine with me, I'm not familiar enough with that code to say what's the best solution.

[tkelman]

@stevengj or anyone else opposed to this? Would be good to fix some (most?) of the package breakage from #19387.

[stevengj]

Shouldn't the decision about whether an object has a shape be the same as for broadcast? e.g. tuples and arrays have shapes?

[pabloferz]

If that were there consensus we would have [(1,); (2,); (3,)] == [1, 2, 3] and [(1,) (2,) (3,)] == [1 2 3]. I'd figure some people would find it surprising/annoying needing to write Tuple{Int,Int}[(1,1) (1,2); (2,1) (2,2)] to avoid getting back

4×2 Array{Int64,2}:
 1  1
 1  2
 2  2
 1  2

though, I wouldn't have a problem with this. I was just trying to get back the original behavior for now (as there are some packages relying on it that are broken right now).

If people feel that we should change the behavior here, I can accommodate whatever is decided. Alternatively, we could open an issue to discuss this and make the appropriate changes in another PR.

[nalimilan]

Indeed that doesn't sound like the most common need. I'd say let's restore the previous behavior, and discuss this more thoroughly in another issue.

[pabloferz]

@tkelman I believe this together with the first three commits from #19387 could be backported to 0.5.1.

[tkelman]

Is it critical? I don't know how closely we tracked whether this fixed every single one of the package issues from #19387, and I'm trying to get the backports wrapped up.

[pabloferz]

I'm pretty confident that this fixed the issue from #19387. But it's certainly not critical, so it wouldn't be too bad if this doesn't make it into 0.5.