-
-
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
array reductions (sum, mean, etc.) and dropping dimensions #16606
Comments
I agree with you. Perhaps the default behavior could be to drop (for consistency with A[1,:]) and have a keyword argument on reduction functions to keep dims. |
Having a keyword argument affect the type (i.e. number of dimensions) of the result would make this function type unstable. |
The way to keep the dimensions is to use a range, i.e. |
I am aware that |
My comment was a reply to @BobPortmann's comment, not to the general issue. |
I think this was settled in #13612 Edit: "settled" is a bit too strong |
I am aware of the way |
#18271 is the strongest argument I've seen yet in favor of dropping dimensions from reductions. It's a pity we don't have an analog of Alternatively we could switch the reduction functions so that they required a boolean tuple for |
This seems not-fully-decided, so moving to 1.0. |
+1 for making reductions and |
Discussion on Discourse: https://discourse.julialang.org/t/sum-squeeze-as-one-function/2471 |
FWIW, just got bitten by this: I was expecting something like |
Half-baked idea prompted by #22000 (comment): we could pass reduction functions through the indexing syntax in order to specify which dimensions get reduced. E.g., |
That's an appealing approach, which reminds me of the |
Huh, that's a pretty cool syntax idea. |
i guess it's a dumb question, but why don't we differ the behaviour in the same way as we do for array indexing? Edit: rethinking my idea showed me why: for indexing colon style is used per dimension while for sum the colon style is used across dimensions Edit2: #16606 (comment) sounds indeed very clever |
I have proposed, elsewhere |
Since we're unlikely to change this in 1.0 and adding |
The argument against the keyword form is not that keyword arguments are slow (when that applied, dims also wasn't a keyword, so I don't think that was every the problem), it's that you have to add that keyword to every single reducer everywhere. So we can do this in Base but any custom reducers out there will not have support for the same pattern. It's just weird to have to implement the same completely identical functionality over and over again, even though there's a completely composable solution that requires no code changes at all, just a new function. Also, no one has addressed the type stability argument: changing return type based on a keyword argument is not generally a good idea. |
Yeah, I think the arguments for sum(A; dims=1) do x
x > 0 ? log(x) : x
end and in some ways in would be nice to be able to write (something like) @dropdims sum(A; dims=1) do x
x > 0 ? log(x) : x
end rather than e.g. dropdims(sum, x -> x > 0 ? log(x) : x, A, dims=1) or g(x) = x > 0 ? log(x) : x
dropdims(sum, g, A, dims=1) But all are an approvement over the current dropdims(
sum(X; dims=1) do x
x > 0 ? log(x) : x
end,
dims=1
) |
In #32860 we are discussing possible syntaxes for a[., ., :] which is lowered to Axed{(1, 2)}(a) where struct Axed{dims, T}
x::T
end (probably it should be This way, dropdims(sum(a[., ., :])) can implement a type-stable version of dropdims(sum(a; dims=(1, 2)); dims=(1, 2)) |
The argument that adding Here's a quick attempt at a macro which allows using MacroTools
macro dropdims(ex) # doesn't handle reduce(...; dims=..., init=...) etc.
MacroTools.postwalk(ex) do x
if @capture(x, red_(args__, dims=d_)) || @capture(x, red_(args__; dims=d_))
:( dropdims($x; dims=$d) )
elseif @capture(x, dropdims(red_(args__, dims=d1_); dims=d2_) do z_ body_ end) ||
@capture(x, dropdims(red_(args__; dims=d1_); dims=d2_) do z_ body_ end)
:( dropdims($red($z -> $body, $(args...); dims=$d1); dims=$d2) )
else
x
end
end |> esc
end
@dropdims begin
a = sum(ones(3,7), dims=2)
b = sum(10 .* randn(2,10); dims=2) do x
trunc(Int, x)
end
(a isa Vector, b isa Vector) # (true, true)
end |
Thinking about " |
If this |
I should have clarified that it is not the case. My model of ndims(a::Axed) = ndims(a.x)
size(a::Axed) = size(a.x)
getindex(a::Axed, I...) = getindex(a.x, I...)
|
Ah, I didn't realise you were thinking of sum(p .* log.(p[:,.] ./ q) Here In the other thread I suggested that the @views b[:, .] .= f.(a[:, .]) # foreach((x,y) -> x .= f(y), eachcol(b), eachcol(a))
c[:, .] := f.(a[:, .]) # c = mapslices(f, a, dims=1) |
I implemented a function sum(along(x, &, &, :)) which is equivalent to julia> using ReduceDims
julia> x = ones(2, 3, 4);
julia> y = sum(along(x, &, :, :)) # lazy sum(x; dims=1)
mapreduce(identity, add_sum, along(::Array{Float64,3}, (1,)))
julia> copy(y) isa Array{Float64, 3}
true
julia> dropdims(y) isa Array{Float64, 2} # type stable
true
julia> zeros(1, 3, 4) .= y; # non-allocating
julia> zeros(3, 4) .= y; # automatically drop dims With #31020, you can also fuse it with broadcasting: julia> using LazyArrays: @~ # need LazyArrays until #19198 is resolved
julia> y = sum(along(@~(x .+ 1), :, &, :))
mapreduce(identity, add_sum, along(::Base.Broadcast.Broadcasted{Base.Broadcast.DefaultArrayStyle{3},Tuple{Base.OneTo{Int64},Base.OneTo{Int64},Base.OneTo{Int64}},typeof(+),Tuple{Array{Float64,3},Int64}}, (2,)))
julia> zeros(1, 3, 4) .= y; [1] There is also along(x::Array{<:Any, 2}, &, +, :) === along(x, &, :)
along(x::Array{<:Any, 3}, &, +, :) === along(x, &, &, :)
along(x::Array{<:Any, 4}, &, +, :) === along(x, &, &, &, :)
along(x::Array{<:Any, 2}, :, +, &) === along(x, :, &)
along(x::Array{<:Any, 3}, :, +, &) === along(x, :, :, &)
along(x::Array{<:Any, 4}, :, +, &) === along(x, :, :, :, &) |
@mcabbott I think something like The reason behind But I agree that iterator-of-iterators approach is quite useful especially because it lets you use functions that are not aware of |
This simple definition makes it easier to write reductions that drops the dimensions over which they reduce. Fixes JuliaLang#16606, addresses part of the root issue in JuliaLang#22000.
Can we do something about this? Would be great if we can replace |
The better approach here is probably to use |
from a performance side
or using
For
|
2.0 should probably remove |
Timing this: julia> let A = rand(1000, 1000);
B = @btime sum($A; dims=2)
C = @btime sum(eachcol($A)) # adds whole slices, won't work for prod
D = @btime map(sum, eachslice($A; dims=1, drop=false))
B ≈ reshape(C,:,1) ≈ D
end
min 256.542 μs, mean 264.496 μs (5 allocations, 8.02 KiB)
min 681.833 μs, mean 13.722 ms (1000 allocations, 7.74 MiB)
min 1.453 ms, mean 1.464 ms (1 allocation, 7.94 KiB)
true
julia> VERSION
v"1.10.0-DEV.100" |
Hmm, that's very weird. The relative performance seems worse now that the type instability is fixed, despite using way fewer allocations? |
(Why the quote?) You'll see different timings on different sizes & dimensions. One issue my example highlights is that some ways of slicing are very cache-unfriendly. You could overload say julia> sum(CUDA.rand(10, 10), dims=1)
1×10 CuArray{Float32, 2, CUDA.Mem.DeviceBuffer}:
5.94849 5.52325 5.60857 5.23081 5.4453 3.76833 6.61417 2.87759 5.43322 4.84982
julia> map(sum, eachslice(rand(Float32, 10, 10), dims=2, drop=false))
1×10 Matrix{Float32}:
5.65775 5.5501 5.40247 3.77431 4.48232 5.90293 6.42368 5.52671 5.20836 5.30931 Another is how this ought to be written: julia> sum(ones(2, 7); dims=2, init=0.0+3im)
2×1 Matrix{ComplexF64}:
7.0 + 3.0im
7.0 + 3.0im There is an issue a bit like this with the magic fast path for |
We could make Is this a problem with |
Following this discussion on julia-users: https://groups.google.com/forum/#!topic/julia-users/V-meqUZtb7k
I find the fact that
sum(A,1)
andA[1,:]
have different shapes in 0.5 counter intuitive. I am not aware of other languages for which the equivalent expressions have different shapes. It leads to unexpected results when computing e.g.A[1,:] ./ sum(A,1)
. At least in the case ofsum
, it seems likesum(A,1)
should be equivalent toA[1,:] + A[2,:] + A[3,:] + ...
However, as @timholy pointed out, this behavior can be useful for broadcasting with expressions like
A ./ sum(A,1)
.For me, having the default slicing and array reduction behavior be consistent seems the natural choice, meaning that
sum(A,1)
should drop the first dimension. But I understand the attractiveness of not dropping dimensions for broadcasting. What are others' thoughts?The text was updated successfully, but these errors were encountered: