-
-
Notifications
You must be signed in to change notification settings - Fork 5.5k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Allow reduce(cat(dims=4), A)
, with efficient method for simple cases
#37196
Conversation
The slightly weird feature of |
One question would be whether the natural default should be cat along the highest input dimension, or to add one new dimension? I can see the argument for the latter, and was initially going to support it. But your second point makes me think the former would be a more consistent API. If someone desired concatenation along the next-highest dimension then, they could reshape the result or reshape at least one of the inputs to have a 1-length extra dimension. |
For a vector of arrays, With a version of Base.Fix2 holding keywords, something like How strange would |
I worry that some people might also think that this has a different "obvious" behavior: julia> cat([1,2,3,4], [1,2,3])
ERROR: UndefKeywordError: keyword argument dims not assigned
Stacktrace:
[1] cat(::Vector{Int64}, ::Vararg{Vector{Int64}, N} where N)
@ Base ./abstractarray.jl:1725
[2] top-level scope
@ REPL[4]:1 Because the two arrays have different lengths, "clearly" I want to concatenate along dimension 1. Yet with this PR I will get the strange error message ERROR: DimensionMismatch("mismatch in dimension 1 (expected 4 got 3)") That error message should probably be clarified anyway, but it has to be thrown after the default for So on balance I am against this change. |
Good point, to think about someone who doesn't have Maybe it should not provide a default for |
I could just generate a 2-dimensional example of the same thing. My underlying point is that successful mind-reading usually depends on which mind, in which state, you're trying to read, and the diversity of minds is impressively large. |
dims=ndims+1
for catreduce(cat, A)
which infers dims=ndims+1
I agree that some mind-reading is necessary to guess the ways in which people may think something ought to work, despite documented behaviour. (Otherwise it would be enough to give every function a random string of letters as a name.) I am persuaded that letting What would still be useful, IMO, is an efficient method like If julia> reduce(cat, [ones(2), rand(3)])
ERROR: ArgumentError: expected arrays of consistent size, got (Base.OneTo(3),) for argument 2, compared to (Base.OneTo(2),) for the first
julia> reduce(cat, [ones(2), rand(2,2)])
ERROR: MethodError: no method matching _typed_cat(::Type{Float64}, ::Vector{Array{Float64, N} where N}) |
I'd be all in favor of |
Well the crude solution is @eval Base begin
cat(; dims) = Fix1(_cat, dims)
(f::Fix1)(ys...) = f.f(f.x, ys...)
end
reduce(cat(dims=4), [fill(i, 4,3,2) for i in 1:5]) # this PR's case, done inefficiently
reduce(cat(dims=(2,3,4)), [fill(i, 4,3,2) for i in 1:5]) Presumably an extension of While the first of these could be done by the efficient Adding such things later would not conflict with the simple |
Perhaps not in a technical sense, but if we're going to add a mechanism to explicitly specify the dimension, why add a somewhat-arbitrary default choice at all? Currently |
I guess we differ on how arbitrary this is. In my mind:
Concatenating arrays of varying size has to be done along the dimension which varies, and that can't be seen in the type, and often won't result in this simple storage packing. This has to be specified. But wanting to concatenate many 3-arrays along (say) a varying-sized 2nd dimension is a pretty rare request. Maybe concatenating many diagonal blocks into a matrix |
It seems like the fundamental issue, @mcabbott, is that to achieve what you want, you really need a preliminary step to figure out the dimensions, which I'm not sure you can get away without a preparatory step that either determines the dimensions to concatenate along, and so can be explicitly partially applied to the |
I'm not sure I follow about there being multiple steps, sorry. The operation performed by There is a zoo of other possible ways to map many arrays into one. Some of them are covered by the existing In numpy, the function >>> arrays = [np.random.randn(3, 5, 7) for _ in range(11)]
>>> np.stack(arrays).shape # default is axis=0
(11, 3, 5, 7)
>>> np.stack(arrays, axis=1).shape
(3, 11, 5, 7)
>>> np.stack(arrays, axis=3).shape
(3, 5, 7, 11) Its behaviour with non-default keywords is |
I begin to think maybe the Edit -- this causes |
This mostly looks good. I'd be willing to see the result of It still contains the mind-reading, though. Since you really seem to want that, I think this needs to be discussed at triage. |
reduce(cat, A)
which infers dims=ndims+1
reduce(cat(dims=4), A)
, with efficient method for simple cases
OK, sounds good. I've updated the first post (and title) to hopefully make things clearer to anyone tuning in now. |
To bump this, here's an example from a few days ago (on slack). The outer container is 2-dimensional, and
Python's |
Did this ever get discussed at triage? Keep in mind that my only objection is the mind-reading---I actively dislike it, but I wouldn't stand in the way if it's popular with a wider audience. But with just the two of us discussing this, it's hard to know how others feel. If you split the mind-reading into a separate PR, I think we can merge the rest without needing triage. Then that part can get discussion on its own schedule. |
I am a bit worried about changing the behavior of 0-argument |
Worth mentioning that this operation is slightly less awkward with my PR #33697
That's because it infers how big the final array should be from the array elements. Is there a technical term for "promoting" an array of arrays to a single multdimensional array by adding a dimension? A function with an appropriate name could map to the above. EDIT: For lack of something better, |
We didn't really have time to talk about this very thoroughly last triage, but one thing that was proposed was maybe introducing a functor |
OK, that would avoid messing with zero-arg reduce(Cat(4), [ones(2,3) for _ in 1:5]) # size (2, 3, 1, 5) # M=1 outer, N=2 inner
reduce(Cat(), [rand(3,5) for μ in 1:7, ν in 0:10]) # size (3, 5, 7, 11) # M = N = 2 Maybe another possibility is to consider concatenate([ones(2,3) for _ in 1:5]; dims=4) Then it would make sense for this also to be used instead of A third idea might be to pass reduce(cat, [ones(2,3) for _ in 1:5]; dims=4) # size (2, 3, 1, 5), perhaps
reduce(+, [rand(3,5) for μ in 1:7, ν in 0:10]; dims=1) # size (7, 1), containing arrays (3, 5) |
I think such methods of |
I was initially in favor of a default "higher dimension if not specified" rule (and didn't really think of it as mind reading), but I'd disagree with the implementation here. That is, I'd expect the following to be of size (3, 5, 77).
On triage, others thought that it'd automatically append on one of the existing dimensions... so perhaps this is indeed mind reading. |
Thanks all for looking. I guess this is evidence that tying this to the name But the functionality issue remains: combining a vector of N-dimensional arrays into one N+1 array (or worse, and M-dimensional container into an M+N-array) is at present quite awkward. Except for For inspiration, #32310 will allow Or Edit:
|
Updated:
This adds an efficient method to do things like
reduce((x,y) -> cat(x,y,dims=3), A)
for a vector of matrices. This can be called asreduce(cat(dims=3), A)
, where keyword-onlycat
now returns aFix1
function.reduce(cat(dims=1), A)
will also dispatch toreduce(vcat, A)
, etc.The new internal
_typed_cat
function handles stacking an M-array of uniform-size N-arrays such thatB[I,J] == A[J][I]
, and can be called byB = reduce(cat, A)
. The explicitreduce(cat(dims=N+1), A)
dispatches to this when M=1. As doesreduce(cat(dims=N+2), A)
by inserting a trivial dimension, i.e.B[I,1,j] == A[j][I]
. For example:Other concatenations such as
reduce(cat(dims=(1,2)), A)
will simply proceed pairwise. It would be possible to add a more efficient method, later, if there is a need.Earlier versions of this PR also added a method for
cat(A,B,C)
to infer dimensions, but (1) perhaps that's too confusing, and (2) it doesn't fit well with the abovereduce
, as after the first reduction,ndims
has changed, and (3) anyway it doesn't allow anything new to be done easily. So this has been removed. Which means thatreduce(cat, A)
on anything that isn't a uniform M-array of N-arrays will still be an error.In the beginning, this said:
This lets you omit the
dims
keyword incat
, when acting on arrays of consistentndims
. One reason to do so is thatreduce((x,y) -> cat(x,y,dims=3), A)
can then be writtenreduce(cat, A)
, which can have an efficient method like existingreduce(hcat, A)
. (They agree for a vector of vectors.)Needs tests, and a less crude implementation than usingBut first, is this a good idea?reduce(hcat, vec.(A))
whenndims(first(A)) > 2
.