-
Notifications
You must be signed in to change notification settings - Fork 41
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
Generalize eachslice #462
Generalize eachslice #462
Conversation
Codecov Report
@@ Coverage Diff @@
## main #462 +/- ##
==========================================
+ Coverage 89.46% 89.64% +0.17%
==========================================
Files 37 38 +1
Lines 2601 2636 +35
==========================================
+ Hits 2327 2363 +36
+ Misses 274 273 -1
📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more |
I don't see any problems with this; |
I'll do some benchmarking to see how fast |
Quick benchmark: julia> using Revise, Random, BenchmarkTools
julia> using DimensionalData
julia> Random.seed!(42);
julia> da = DimArray(randn(5, 2, 10), (X(1:5), Y(1:2), Z(1:10)));
julia> dims = (Z, X);
julia> dims2 = (:Z, :X);
julia> dims3 = Dimensions.dims(da, (Z, X));
julia> foo(x, dims) = eachslice(x; dims=dims); using julia> @btime foo($da, $dims);
4.735 μs (55 allocations: 2.89 KiB)
julia> @btime foo($da, $dims2);
2.819 μs (37 allocations: 2.48 KiB)
julia> @btime foo($da, $dims3);
2.876 ns (0 allocations: 0 bytes) this PR: julia> @btime foo($da, $dims);
3.120 μs (34 allocations: 2.00 KiB)
julia> @btime foo($da, $dims2);
2.040 μs (23 allocations: 1.84 KiB)
julia> @btime foo($da, $dims3);
2.875 ns (0 allocations: 0 bytes) So what this PR does is slightly faster for More importantly, when |
Ok, that makes sense. |
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.
LGTM
I do actually read your PRs, I just don't find much to change.
# works for arrays and for stacks | ||
function _eachslice(x, dims::Tuple) | ||
slicedims = Dimensions.dims(x, dims) | ||
return (view(x, d...) for d in DimIndices(slicedims)) |
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 is so succinct 😍
This PR:
eachslice
forAbstractDimArray
to accept multiple dimensions and return a generator with the same size and axes as the provided dimensionseachslice
forAbstractDimStack
using the same implementation (fixes Adding eachslice for AbstractDimStack #455)AbstractDimArray
to return aSlices
for v1.9 (this doesn't really make sense for stacks)Note that #418 also would add
eachslice
support for v1.9. The implementation in this PR avoidsinvoke
. @ParadaCarleton you've probably thought about this more than I have. Any reason to not use this approach?