-
Notifications
You must be signed in to change notification settings - Fork 43
Try converting indexes once and then defer to the parent #164
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
Conversation
AxisArrays has to handle many different index types, but it can't handle types defined in other packages. This provides for a fallback mode in which AxisArrays tries to convert "fancy" index types into "regular" ones, but if this fails then it assumes this is something the parent knows how to handle.
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 looks reasonable to me. So the call tree for getindex(A::AxisArray, w::WeightedIndex)
(manually inlining) is
getindex(A, w)
->
getindex_converted(A, to_index(A,(w,))...)
->
getindex(A, to_index(A,(w,))...) # since to_index returns an `Idx`
->
AxisArray(A.data[to_index(A,(w,))...], reaxis(A, to_index(A,(w,))...))
What about the fallback path? Do you have an example/test for a kind of specialized index that AxisArrays
can't/shouldn't handle?
@propagate_inbounds getindex_converted(A, idxs::Idx...) = A[idxs...] | ||
@propagate_inbounds setindex!_converted(A, v, idxs::Idx...) = (A[idxs...] = v) | ||
|
||
@propagate_inbounds getindex_converted(A, idxs...) = A.data[idxs...] |
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.
Do you need to re-wrap the Axes
into the result somehow here (if an array is returned) or is this just a case of "AxisArrays doesn't understand the type of the indices, so all bets are off"?
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.
Yep. If AxisArrays doesn't know how to handle the index, then I don't think it can make sensible decisions about whether to return the axis wrapper or not. The particular case I was worried about returns a scalar, so of course it's biased to do that. If it should return more then I think one needs some glue code (i.e., via Requires or similar).
Not quite.
The test I submitted is one such example. Or is there something that's not covering? |
Sorry, my mistake not reading closely enough. This looks good to me then. |
I was impressed you asked. It's quite difficult to mentally execute dispatch paths as complex as those in this package! Thanks for the review. I'll leave it open until next Monday; aside from giving others a chance to point out concerns, I'm on vacation and I'd rather not have to deal with fallout from anything I break 😉. |
Ah darn, I forgot about this one or I would have put it in the release. Never mind though, version numbers are cheap :-) |
AxisArrays has to handle many different index types, but it can't handle types defined in other packages. This provides for a fallback mode in which AxisArrays tries to convert "fancy" index types into "regular" ones, but if this fails then it assumes this is something the parent knows how to handle.
Fixes the following error:
I want to give folks a little time to complain about anything this breaks, so I won't merge in the next week. But @ChantalJuntao you can check out this branch.