-
-
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 Slices array type for eachslice/eachrow/eachcol #32310
Conversation
The way I handle |
Also, instead of
|
Yes, this was just meant to be an illustrative example. |
Figuring out the appropriate |
Honestly, though, maybe it makes sense just to copy-paste the code from JuliennedArrays and define
? I've seen an uptick in attention to it. Maybe its at long last time to use a generated function to support numbered dims... I'll give it a shot. |
Cool. I think there are advantages in preserving the |
Well, just added the capability of processing |
I did consider it, but there were a few things that made me a bit uneasy: e.g. the use of
I agree: the main challenge in doing that is figuring out the |
One thing that is perhaps worth noting about my PR is that the order of
happy to hear people's thoughts on this. |
Can you give more detail? I'll need to figure out a way to fix JuliennedArrays if so. |
Well, you're using |
bump? :) |
@mbauman any thoughts? |
Ref #32940 |
This looks great. My only hesitation is that we worked quite a bit on performance in #29749 — this looks like it should be good, but have you done any spot checks? |
base/abstractarraymath.jl
Outdated
@@ -449,9 +480,10 @@ See also [`eachrow`](@ref), [`eachcol`](@ref), and [`selectdim`](@ref). | |||
This function requires at least Julia 1.1. | |||
""" | |||
@inline function eachslice(A::AbstractArray; 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.
Would it be possible to have an intermediate step to allow overloading of dispatch on dims
? Motivated by dims=:μ
type things in NamedDims.
@inline function eachslice(A::AbstractArray; dims) | |
eachslice(A::AbstractArray; dims) = _eachslice(A::AbstractArray, dims) | |
@inline function _eachslice(A::AbstractArray, dims) |
Is there any interest in reviving this? |
Once you have the concept in place I would gladly review it. I can see it being beneficial to have this be in a more general interface package, but I can't make any promises as to whether it will be integrated directly into the ArrayInterface repo until everyone has a chance to review the actual code. |
Thanks - I definitely want to include everyone interested in the design/discussion, so we end up with something with broad support in the Julia community. |
@torfjelde and @ToucheSir, I hope such a common sliced array interface can also be very useful regarding https://github.com/TuringLang/Bijectors.jl/discussions/178 . |
Maybe add to documentation an example to use the new multiple dimensions option which was added? |
ArrayInterfaceCore is released, so instead of prototyping the sliced array interface as a standalong temporary package, I'll prototype it as an ArrayInterfaceCore PR - Ok @Tokazama? |
That seems fine. We still need to set some standards for what goes in core vs other modules because right now there are some things that are left out because of load time issues and not because they don't fit conceptually there. |
Thanks @Tokazama I'll give it a go and try to keep it extremely lightweight. |
If we have to reconsider which part of the repo it goes in or rework it a bit that shouldn't be too big of a deal. There are a lot of moving pieces right now and I would hate for that complexity to discourage meaningful contributions. |
@Tokazama I'll try to have a PR draft ready within the next days. |
The current implementation of julia> A = eachslice(rand(2,3), dims = 2, drop = false); axes(A)
(Base.OneTo(1), Base.OneTo(3))
julia> A[2,3]
2-element view(::Matrix{Float64}, :, 3) with eltype Float64:
0.5964895828663271
0.4755380804162148 Should we add |
Relatedly, should the new axis be julia> using OffsetArrays
julia> oa = OffsetArray([1 2 3; 4 5 6], 7, 8)
2×3 OffsetArray(::Matrix{Int64}, 8:9, 9:11) with eltype Int64 with indices 8:9×9:11:
1 2 3
4 5 6
julia> sum(oa, dims=1) # trivial axis is 8:8
1×3 OffsetArray(::Matrix{Int64}, 8:8, 9:11) with eltype Int64 with indices 8:8×9:11:
5 7 9
julia> eachslice(oa, dims=2, drop=false) # trivial axis is 1:1
1×3 eachslice(OffsetArray(::Matrix{Int64}, 8:9, 9:11), dims = 2, drop = false) of 2-element slices with eltype Int64 with indices Base.OneTo(1)×OffsetArrays.IdOffsetRange(values=9:11, indices=9:11):
[1, 4] [2, 5] [3, 6] |
Ah, good catch! Can you open an issue (or a PR)? |
It should probably inherit from the parent? |
I was wondering about that too - it would be more natural to inherit indexing from the parent, I think. |
Another thing, while we're taking fixes and changes: I'm currently building the generic sliced array interface we've been talking about (CC @Tokazama). The idea is for code to be able to flatten a sliced arrays in either it's "native" or a specific order of dimensions (e.g. inner dims first or outer dims first), so that efficient computations (e.g large linalg ops) can be performed on it, before (possibly) re-slicing the result. On thing that I'm struggling with is that the current slices implementation is not fully type stable in respect to dimension order: We currently have typeof(eachslice(A, dims = (1, 2))) != typeof(eachslice(A, dims = (1, 3))) but typeof(eachslice(A, dims = (1, 2))) == typeof(eachslice(A, dims = (2, 1))) So it's impossible to decide whether a While Julia v0.9 isn't branched yet - could we still change |
In the ArrayInterface code you're using you can use |
WIthin ArrayInterface, yes, but it won't be in the @simonbyrne , this is your baby though, what do you think? |
Sure! @simonbyrne , would you be fine with me making the slicemap part of the |
#44538 would present another way to make the slicemap inferable (CC @Tokazama ). @simonbyrne, do you have any preferences here? |
I'm assuming it would work to put the slicemap in the type just as well as it works to put the permutation in |
Indeed, especially since we want to be able to pass such things around. |
eachslice
,eachrow
,eachcol
(introduced in #29749) now return anSliceArray
object (along withRows
/Columns
aliases). The main benefit is that it will allow dispatch on the iterator to provide more efficient methods, e.g.This will encourage the use of
eachcol
/eachrow
to resolve ambiguities in user-facing APIs, in particular, the "observations as rows vs columns" problem in the statistics/ML packages.This also makes
eachslice
work over multiple dimensions.TODOs:
eltype
: I'd appreciate some help on how best to do this.cc: @nalimilan @mschauer @ararslan @kleinschmidt @mbauman @andyferris @bramtayl @arnavs