Skip to content
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

unbroadcast for tuples #977

Merged
merged 2 commits into from
May 22, 2021
Merged

unbroadcast for tuples #977

merged 2 commits into from
May 22, 2021

Conversation

mcabbott
Copy link
Member

Fixes #975

@@ -55,6 +56,7 @@ unbroadcast(x::AbstractArray, x̄) =
unbroadcast(x::Number, x̄) = accum_sum(x̄)
unbroadcast(x::Tuple{<:Any}, x̄) = (accum_sum(x̄),)
unbroadcast(x::Base.RefValue, x̄) = (x=accum_sum(x̄),)
unbroadcast(x::Tuple, x̄) = trim(x, length(x) == length(x̄) ? x̄ : accum_sum(x̄; dims=2:ndims(x̄))) # case length(x) > 1
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I wonder if we can use dispatch on abstractarrays instead of this branching

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What this should avoid is summing a 9x1 matrix over the 2nd dimension, and hence making a copy. whether this can plausibly matter for arrays short enough to become tuples, I don't know. The function does the same thing for arrays (just above) where it can matter:

julia> @btime sum($(rand(100,1,100)); dims=2);
  3.713 μs (8 allocations: 78.31 KiB)

julia> @btime sum($(rand(100,2,100)); dims=2);
  5.229 μs (8 allocations: 78.31 KiB)

julia> @btime reshape($(rand(100,1,100)), (100,100));
  17.117 ns (1 allocation: 64 bytes)

@CarloLucibello CarloLucibello merged commit bcc3921 into FluxML:master May 22, 2021
@mcabbott mcabbott deleted the tuplecast branch May 22, 2021 11:15
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

broadcast involving NTuple errors no method matching unbroadcast(::Tuple{… AND possible fix?
2 participants