Skip to content
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

Clarify the exact bounds of what is required for slices #138

Merged
merged 3 commits into from
Sep 20, 2021

Conversation

asmeurer
Copy link
Member

@asmeurer asmeurer commented Mar 9, 2021

@alextp @apaszke you originally requested this at #46 (review). Can you clarify if this is inline with what you are looking for? @shoyer I seem to remember you also had opinions on this, but I could be misremembering.

Personally, I'm a little confused by this. If you know the shape of the array, you can always statically determine the shape of a sliced axis, regardless of what the slice looks like. Conversely, if the shape is only known symbolically, the sliced axis in general is also indeterminate. This is not just the case for bounds clipping. The size of the first axis in something like a[:-1] is not a fixed number. It's always 1 less than the size of the first axis of a (slices like that are very common in practice). So I think I may be missing something from this discussion.

Even so, every valid array slice is representable as a slice with the given conditions listed here, so this shouldn't affect expressibility.

For context, the reason this matters is that NumPy array API implementation, we want to make something that only allows those things that are explicitly required by the spec, so that people can use it as a test to see if their code will work for every API compatible implementation. So in particular, we want to restrict the indexing in numpy._array_api so that it doesn't allow indices that may not be supported by other valid array API implementations. So it's important to be precise about exactly what is required by the spec.

@alextp
Copy link

alextp commented Mar 9, 2021

If you know the shape of the array, you can always statically determine the shape of a sliced axis, regardless of what the slice looks like.

I wish!

An example is arange(10)[x:] for a dynamic x, where the shape of the result is [10 - x] for 0 <= x < 10 and [x] for -10 <= x < 0.

But I think the issue Adam and I were more worried about (at least I was) is with operations like np.take which index into an array given the indices of another array. On accelerator devices it's hard to enforce that np.take(x, y) should fail immediately if y is out of range (think np.take(np.arange(10), [-1, 11])). So some implementations will truncate out of range indices to in-range indices and others put NaNs in the invalid coordinates instead of failing immediately.

I looked over the text of the PR and it is mostly an improvement over the previous text, though.

@asmeurer
Copy link
Member Author

If you know the shape of the array, you can always statically determine the shape of a sliced axis, regardless of what the slice looks like.

I mean if both the shape and exact slice values are known. If either is symbolic, the shape will in general depend on those symbolic values (quite often a piecewise expression, even without bounds clipping, because of the way negative index semantics work).

asmeurer added a commit to data-apis/array-api-tests that referenced this pull request Mar 18, 2021
@rgommers rgommers added the Maintenance Bug fix, typo fix, or general maintenance. label Mar 20, 2021
@rgommers rgommers force-pushed the main branch 3 times, most recently from 0607525 to 138e963 Compare April 19, 2021 20:25
Copy link
Contributor

@kgryte kgryte left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM barring a couple minor copy changes.

spec/API_specification/indexing.md Outdated Show resolved Hide resolved
spec/API_specification/indexing.md Outdated Show resolved Hide resolved
asmeurer and others added 2 commits June 14, 2021 14:22
Co-authored-by: Athan <kgryte@gmail.com>
Copy link
Contributor

@kgryte kgryte left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM.

@kgryte kgryte merged commit 12c2bd9 into data-apis:main Sep 20, 2021
cr313 added a commit to cr313/test-array-api that referenced this pull request Apr 19, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Maintenance Bug fix, typo fix, or general maintenance.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants