-
-
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
Add stack(array_of_arrays)
#43334
Add stack(array_of_arrays)
#43334
Conversation
As I stated in #21672, a concern is that |
Re names, first English: If you want to "stack" some papers, or some hay, then you don't put the pieces together on their long dimensions as that would fall over. Maybe it becomes less clear if you stack textbooks. Whereas "concatenate" seems to come from liturgy, to mean what it does for strings, or architecture, like Python's numpy.stack has this meaning, and explicitly distinguishes this "new axes" vs. In Julia, if this is the inverse of My objection to |
Could we also add a two-arg version with a function argument? (Could just be implemented as |
Maybe |
I'm sure I've suggested this
[Edit] Since this thread is now very long, I've enlarged this slightly. There are examples below where the point was raised again. |
base/abstractarray.jl
Outdated
julia> stack("julia") do c | ||
(c, c-32) | ||
end | ||
2×5 Matrix{Char}: | ||
'j' 'u' 'l' 'i' 'a' | ||
'J' 'U' 'L' 'I' 'A' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I added the stack(f, iter)
method suggested.
Notice that this allows f
to return tuples, and treats them just like vectors. This is not how cat
behaves, but if it's a different verb then perhaps we have more freedom. The reason to want this is that, if your function always returns (say) 3-vectors, then this might let you avoid allocating many small vectors.
You could go further and make stack([(1,2), (3,4)])
into reinterpret(reshape, Int, [(1,2), (3,4)])
. But that's perhaps a surprising return type.
Edit: The analogy to Iterators.flatten
is another reason to think tuples should be allowed. Having vec(stack(iter)) == collect(Iterators.flatten(iter))
seems like a simple and consistent rule.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
However, this has type-stability problems right now? No, I was testing wrong. Corrected version:
julia> x = randn(3, 1000);
julia> @btime stack(eachcol($x));
min 4.774 μs, mean 7.023 μs (4 allocations, 23.58 KiB. GC mean 12.95%)
julia> ft(x) = @inbounds (x[1], x[2]/2, x[3]^3);
julia> fv(x) = @inbounds [x[1], x[2]/2, x[3]^3];
julia> @btime stack(fv, eachcol($x));
min 21.125 μs, mean 25.090 μs (1004 allocations, 101.70 KiB)
julia> @btime stack(ft, eachcol($x)); # making tuples not vectors
min 4.768 μs, mean 6.436 μs (4 allocations, 23.58 KiB)
julia> @btime mapslices(fv, $x, dims=1); # alternative (although this can be improved)
min 222.333 μs, mean 237.788 μs (7502 allocations, 266.73 KiB)
There is also |
Ref #16708 |
Thanks, I hadn't seen that iterator approach. Perhaps something like that could now be written as mixture of But can it be fast? It seems that julia> xs = [rand(128) for _ in 1:100];
julia> v = @btime collect(Iterators.flatten($xs));
min 55.750 μs, mean 64.555 μs (9 allocations, 326.55 KiB)
julia> v ≈ @btime collect(reduce(vcat, $xs))
min 4.542 μs, mean 13.543 μs (5 allocations, 200.97 KiB)
true |
Yes, it is a tricky thing, that's perhaps why it didn't went anywhere. On the other hand
so |
Indeed, that's pretty fast. (Notice we're penalising The analogy to stack(xs, eachcol(Y)) do x, y
x/y[1], y[2]/y[3], y[4]/x
end Allowing this seems like an advantage of not calling this I think I have a better implementation now, which doesn't make an iterator but does accept them. It deletes all the code dealing with julia> xs = [@SVector rand(3) for _ in 1:100];
julia> m = @btime reduce(hcat, $xs);
min 539.243 ns, mean 843.973 ns (1 allocation, 2.50 KiB)
julia> m ≈ @btime reshape(collect(Iterators.flatten($xs)), 3, :)
min 344.329 ns, mean 637.694 ns (3 allocations, 2.59 KiB)
true
julia> m ≈ @btime reshape(collect(Iterators.flatten(Tuple(x) for x in xs)), 3, :)
min 1.492 μs, mean 2.170 μs (10 allocations, 7.73 KiB)
true
julia> m ≈ @btime stack(Tuple(x) for x in $xs)
min 123.719 ns, mean 417.366 ns (1 allocation, 2.50 KiB)
true Still a bit WIP, am not entirely sure whether to push to this PR or make a separate one. |
Triage likes this. The name situation with DataFrames is tricky, but we do think |
FWIW, julia> using DataFrames
julia> methods(stack)
# 3 methods for generic function "stack":
[1] stack(df::AbstractDataFrame) in DataFrames at /Users/mason/.julia/packages/DataFrames/ORSVA/src/abstractdataframe/reshape.jl:134
[2] stack(df::AbstractDataFrame, measure_vars) in DataFrames at /Users/mason/.julia/packages/DataFrames/ORSVA/src/abstractdataframe/reshape.jl:134
[3] stack(df::AbstractDataFrame, measure_vars, id_vars; variable_name, value_name, view, variable_eltype) in DataFrames at /Users/mason/.julia/packages/DataFrames/ORSVA/src/abstractdataframe/reshape.jl:134 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
OK, I've replaced the implementation here with one which is I think better in every way, except brevity. It will never call append!
, as for iterators of unknown length producing arrays, it seems almost always faster to collect
, and always knowing the size is simpler.
Its goals are:
- Accept an iterator of iterators, not necessarily an array of arrays.
Partly becausestack(f, A)
with a do block sounds like a useful thing. - Be as fast as
reduce(hcat, A)
on a vector of vectors, where they agree.
This seems to be possible without indexingA
, only iterating. - Take a
dims
keyword, such thatstack(eachslice(A; dims); dims) == A
.
Also lines up nicely withselectdim
. This must be an integer (in all 3 functions). - Make OffsetArrays work automatically, by preserving axes.
Unlikecat
, this never needs to put existing axes end-to-end.
I've attached some comments on the implementation below.
base/abstractarray.jl
Outdated
# Iterating over an unknown length via append! is slower than collecting: | ||
_stack(dims, ::IteratorSize, iter) = _stack(dims, collect(iter)) | ||
|
||
function _stack(dims, ::Union{HasShape, HasLength}, iter) | ||
S = @default_eltype iter | ||
T = S != Union{} ? eltype(S) : Any # Union{} occurs for e.g. stack(1,2), postpone the error | ||
if isconcretetype(T) | ||
_typed_stack(dims, T, S, iter) | ||
else # Need to look inside, but shouldn't run an expensive iterator twice: | ||
array = iter isa Union{Tuple, AbstractArray} ? iter : collect(iter) | ||
isempty(array) && return _empty_stack(dims, T, S, iter) | ||
T2 = mapreduce(eltype, promote_type, array) | ||
# stack(Any[[1,2], [3,4]]) is fine, but stack([Any[1,2], [3,4]]) isa Matrix{Any} | ||
_typed_stack(dims, T2, eltype(array), array) | ||
end |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This implementation accepts iterators, but demands to know the size and element type before proceeding. If these are unknown, or abstract, it will collect.
If the elements (the slices being stacked) themselves have abstract eltype, then so will the answer. Which matches this:
julia> Iterators.flatten(Any[[1,2], [3,4]]) |> collect
4-element Vector{Int64}:
1
2
3
4
julia> Iterators.flatten([Any[1,2], Any[3,4]]) |> collect
4-element Vector{Any}:
1
2
3
4
I tried some versions which instead promoted the types within individual slices to make the final output concrete. But these were always much slower. Perhaps getting back a Matrix{Real}
is better, a visible sign of a problem you should fix, rather than an invisible slow path.
I thought cat
always promoted, but in fact it varies:
julia> vcat(Any[1,2], Any[3,4])
4-element Vector{Any}:
1
2
3
4
julia> hcat(Any[1,2], Any[3,4])
2×2 Matrix{Int64}:
1 3
2 4
julia> cat(Any[1,2], Any[3,4]; dims=2)
2×2 Matrix{Any}:
1 3
2 4
base/abstractarray.jl
Outdated
_axes(x, ::HasLength) = (OneTo(length(x)),) | ||
_axes(x, ::IteratorSize) = axes(x) | ||
# throw(ArgumentError("cannot stack iterators of unknown or infinite length")) | ||
|
||
# For some dims values, stack(A; dims) == stack(vec(A)), and the : path will be faster | ||
_typed_stack(dims::Integer, ::Type{T}, ::Type{S}, A) where {T,S} = | ||
_typed_stack(dims, T, S, IteratorSize(S), A) | ||
_typed_stack(dims::Integer, ::Type{T}, ::Type{S}, ::HasLength, A) where {T,S} = | ||
_typed_stack(dims, T, S, HasShape{1}(), A) | ||
function _typed_stack(dims::Integer, ::Type{T}, ::Type{S}, ::HasShape{N}, A) where {T,S,N} | ||
if dims == N+1 | ||
_typed_stack(:, T, S, A, (_vec_axis(A),)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Here _axes(x)
generalises axes(x)
to regard HasLength as being like HasShape{1}
. And I've got what's essentially the same thing for ndims
.
Are these safe? Can a HasLength iterator in fact collect to have multiple dimensions?
And if they are safe, can they just be made methods of axes
and ndims
globally?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You could do something like this globally:
Base.size(iter) = _size(IteratorSize(iter), iter)
_size(::HasLength, iter) = (length(iter),)
_size(::HasShape, iter) = map(length, axes(iter))
One reason not to is that types which are iterable but scalar under broadcast have two notions of size. Choosing this one breaks the link between axes
and broadcast
:
julia> axes("str") # previously an error
(Base.OneTo(3),)
julia> axes(collect("str")) # matches this
(Base.OneTo(3),)
julia> "str" .* '+' # scalar under broadcasting
"str+"
julia> collect("str") .* '+'
3-element Vector{String}:
"s+"
"t+"
"r+"
base/abstractarray.jl
Outdated
x1, _ = xit | ||
ax1 = _axes(x1) | ||
N1 = length(ax1)+1 | ||
dims in 1:N1 || throw(ArgumentError("cannot stack slices ndims(x) = $(N1-1) along dims = $dims")) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This forbids e.g. stack([1:3, 2:4]; dims=3)
, which you might hope would make a 3×1×2 Array
like cat([1:3, 2:4]...; dims=3)
. Doing so lets it be type-stable.
base/abstractarray.jl
Outdated
# For `similar`, the goal is to stack an Array of CuArrays to a CuArray: | ||
_prototype(x::AbstractArray, A::AbstractArray) = x | ||
_prototype(x::AbstractArray, A) = x | ||
_prototype(x, A::AbstractArray) = A | ||
_prototype(x, A) = 1:0 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The output array is always B = similar(_prototype(x1, A), T, axes...)
, with the aim of making an array of CuArrays just work. Perhaps not worth the complexity and better to handle there?
Even without that, using say similar(1:0, T, axes...)
instead of Array{T}(undef, ...)
lets this work with OffsetArrays automatically. Preserving axes
not size
seems like the right thing to do.
base/abstractarray.jl
Outdated
function _empty_stack(dims::Integer, ::Type{T}, ::HasShape{N}, A) where {T,N} | ||
# Not sure we should check dims here, e.g. stack(Vector[]; dims=2) is an error | ||
dims in 1:N+1 || throw(ArgumentError("cannot stack slices ndims(x) = $N along dims = $dims")) | ||
ax = ntuple(d -> d==dims ? _vec_axis(A) : OneTo(1), N+1) | ||
similar(_prototype(nothing, A), T, ax...) | ||
end |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Some empty behaviours, most to least specific:
julia> stack(Tuple{Int,Int,Int}[])
3×0 Matrix{Int64}
julia> stack([(1,2,3)])
3×1 Matrix{Int64}:
1
2
3
julia> stack(Matrix{Int}[])
1×1×0 Array{Int64, 3}
julia> stack([[1 2; 3 4]])
2×2×1 Array{Int64, 3}:
[:, :, 1] =
1 2
3 4
julia> stack(Matrix[]) # eltype is abstract, perhaps not ideal?
Any[]
julia> stack([])
Any[]
julia> stack([1,2,3]) # because numbers are iterable
3-element Vector{Int64}:
1
2
3
My thinking re the Tuple handling is partly that SVectors could share these paths. It's possible I haven't thought enough about SMatrices.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Some comments here below about empty behaviour inducing type-instabilities. Even if the collection type is known, without an instance unfortunately you can't get similar
to make one for you, e.g.:
julia> similar([1,2,3], (4,5)) |> summary
"4×5 Matrix{Int64}"
julia> similar(typeof([1,2,3]), (4,5)) |> summary
ERROR: MethodError: no method matching Vector{Int64}(::UndefInitializer, ::Tuple{Int64, Int64})
It's possible that empty cases should all (or mostly?) be made errors. This is what reduce(hcat, Vector{Int}[])
does.
One-month bump? |
Should I read anything into the test failures? They appear the same on re-running, and don't seem to appear on other recent PRs. But they also look unrelated to me, e.g.: :mac: test x86_64-apple-darwin
:linux: test powerpc64le-linux-gnu
|
They should be unrelated, as we are pretty sure that
Let's merge this at this weekend if there's no other objections. |
I have been waiting for a function like this for years. Thank you @mcabbott and the Julia community at large, you all are amazing |
I wouldn't say I object to this, but I feel we have some conversations going on in #21672 and in #45985, #46003 and #44792. The conversations didn't really reach any form of transcendental synthesis or synergistic consensus, in my opinion. A conclusion was apparently reached, though, since we're going ahead and merging this PR. It feels like we're in a kind of patch-merging race, there are different ideas for how this feature, important one in my opinion, should be brought to |
This may not be the end of the story. One possibility is something like |
Please don't :) It's clearly feasible to preserve the array type and eltype better than that. I've put I'd be glad to have this functionality in base, but won't pursue it myself - a package works just as well in practice, and much easier/faster to get done. |
Yes I don't take any position on precise naming (sometimes Base & Iterators agree, sometimes they differ, not sure why) nor on return types (vector not Vector). At present Perhaps someone should make an issue though, since this isn't the right place, |
There's an open issue already, as I have just mentioned. It didn't make a difference. I feel the concerns raised by me and others were brushed away with not enough attention, with little to no participation of core developers. In the meantime I have tried to offer alternative working code, to enrich the conversation and illustrate my points concretely, and I even did a short conference talk that was actually related to the subject. It was all completely ineffective, I guess I didn't do enough. I feel this function lacks a proper semantic understanding, and I'm not convinced it warrants this special implementation independent of I don't mean to criticize the contribution. I'm very interested in it. Maybe I'm wrong and eventually I'll change my mind, who knows. But right now it's not my feeling at all. This is an important feature that the language lacks. It's not such a big deal, reason why there's only now a PR for it. But I don't feel we have achieved the best design. Again, I don't mean to dunk on your work and I'm happy to see progress being done, but I'm not personally satisfied with the direction this is going. Hope I'm wrong. |
I'm sorry, but doesn't |
It does not.
But there is clarity about this. Much thought has gone into the design.
And you didn't try it out, by typing I don't think this is the right level to discuss on this thread, and encourage you to show up on zulip or slack or something less formal. |
OK, well, thanks for pointing out the example to me. I'm sorry I'll probably keep struggling to intuitively understand what the method does. |
This adds a
simpleversion of the functionstack
described in #21672. It generalisesreduce(hcat, vector_of_vectors)
to handle more dimensions, and to handle iterators efficiently.For iterators, like #31644 this handles only cases for which repeatedThus it can be the inverse ofappend!
is the fallback, except that it will also widen the type if necessary. You would need quite a different implementation to handle things likereduce(vcat, ([1 i i^2] for i in 1:10 if i>pi))
, probably collecting first to know where to write the first slice. Because of that, it does not take adims
keyword.eachcol(::Matrix)
oreachslice(::Array{T,3}; dims=3)
but not ofeachrow
.Edit: The current implementation introduced in this message below takes a
dims
keyword. With this,stack(eachslice(A; dims); dims) == A
. Without thedims
keyword, the dimensions of the slice always come first, thus it should obeyvec(stack(xs)) == collect(Iterators.flatten(xs))
. Iterators of unknown length are collected first, as this seems quicker than falling back toappend!
-and-reshape
.Handling iterators also makes it trivial to allow
stack(f, xs) = stack(f(x) for x in xs)
, as suggested below. This seems nice to have with ado
block.It does not currently handle the empty case
, as I'm not sure what it should do. IdeallyReverted to an error because handling weird array types is hard, and my attempt produced type instabilities.stack(empty!([1:5]))
would make a 5×0 Matrix, but that's impossible. At present it makes a 1×0 Matrix. The empty handling might be more complicated than ideal, see below.Should work fine with OffsetArrays, it preserves all axes. Should mostly work with CUDA.
The name unfortunately clashes with
DataFrames.stack
. Other names suggested in #21672 includepack
ornested
; perhaps seeing it as the inverse ofeachslice
might suggest others?