Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
SubIndex
,LinearSubIndex
, andPermutedIndex
types #202SubIndex
,LinearSubIndex
, andPermutedIndex
types #202Changes from 10 commits
6de3ecf
2d5c260
ace2c57
a963ebd
b309e1f
3da21dd
adaad82
d2fef13
62457e0
56bdcf0
58b9d7b
d391ba5
55ca0e5
aa14e0e
af92766
b424766
9f631ad
fc95c21
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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.
Should add
Although given the
getfield(iperm, C)
fallback in the other method, is this somehow not allowed?Not sure what
PermutedIndex
is without looking at the code more closely.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.
Because many
SubArray
s andPermutedDimArray
s can be represented byStridedIndex
, can we haveArrayIndex
for these return aStridedIndex
?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.
The way I'm treating
ArrayIndex
here is a one layer deep index representation. The problem with going straight toStrideIndex
is that whatever is callingArrayIndex(::AbstractArray)
doesn't know how deeply nested the buffer is. I figured the user level interface would be more likelayout(A, accessor) = ArrayIndex(A), buffer(A)
and we could special case for cartesian indexing going straight toStrideIndex
.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.
@chriselrod, should this PR have something like the
layout
method implemented so that it is immediately more useful? I was trying to make a PR that was still functional but didn't create too many new things at once, but I can see how that lack of a fully implemented interface runs the risk of breaking changes in the near future. Alternatively, I could add warnings to the docs that this is experimental, so that we can drop/change stuff without worrying about breaking changes for now.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.
Sure, although why not have these things live in a separate, experimental repo?
Are they part of the array interface, or something other libraries would want to extend?
It's hard for me to assess changes like these if I don't know the vision for their future use.
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.
Hopefully recent commits involving the indexing pipeline make it more clear that these changes allow unique interactions between layers of nested arrays. If we were to have a separate package for this we'd need ArrayInterface.jl to depend on it so that we could complete the indexing interface.
That being said, we don't need to strictly use what I've proposed here. The point of the
ArrayInterface.compose
methods here is to support a simple rule based system for composing simplified/efficient index transformations. I'm sure will need the new types here, but I'm completely open to there being a more optimal way to doing this.