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
Add array indexing specification #46
Add array indexing specification #46
Changes from 23 commits
e6c7cbf
ff6456d
6480a7d
ab737dc
c27783a
a736f70
6eaf90b
11ac377
35ef60e
702cada
ded8acd
40467a8
50c6b1f
bf16eca
d4dcb6a
abcbc31
7dce65a
9d522f1
74943fb
b764e4b
b5ee472
3522698
1c19139
e114e9c
5c34798
74fd644
2831ceb
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.
For devices like GPUs where you might want to allocate the memory for the result of slicing before you know the indices (as the allocation happens on the host and the indices reside on the device) this type of shape dynamism can be problematic. We should discuss what to do here in the same way as we discussed mutability.
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.
Thanks @alextp for reviewing and raising this concern! Would definitely be good to discuss. cc @rgommers
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 this also be an issue for slices bounds that are beyond the array shape (e.g.,
a[0:1000]
wherea
has shape(100,)
)?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.
I think slice clipping could be omitted from the spec. In other words, the spec only specifies what happens for slices where the start or stop are in [-size, size] (where the ends of that interval may or may not be included depending on the different cases of start/stop and negative/positive step).
Slices on Python lists implement "clipping" behavior to the size, which NumPy matches, but this could be something that isn't specified in the spec, and libraries could do something else. It is also possible to "manually" implement clipping in user code, in much a similar way that you can "manually" implement bounds checking (clipping happens first when a slice is computed, see the
a[-100::2]
example here).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.
Both slices and array shapes are usually kept on the host, so I think you should be able to figure out the necessary buffer sizes without any synchronization. Or am I missing something?
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.
@apaszke reading the first comment from @alextp, I think he meant something like
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.
I've removed any bounds checking requirements from the proposal, under the rationale that how an array implementation handles out-of-bounds indices (including within slices) is best left to the implementation.
Based on your example @rgommers and @alextp's comment, the gist of the problem is that I may want to allocate memory for an array having axis size
2
, regardless of whether indices are valid (in-bounds) or not. And if we required that, in this case, out-of-bounds indices unconditionally must result in an array whose axis size is0
, then I won't be able to allocate memory without knowing the actual index values, thus triggering device syncs and triggering a perf cliff.