Skip to content

Clarify types to allow None in indexing #674

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

Merged
merged 1 commit into from
Sep 19, 2023

Conversation

saulshanabrook
Copy link
Contributor

@saulshanabrook saulshanabrook commented Aug 9, 2023

I believe that the indexing specification allows None in a selection tuple, but the current typing does not specify this. I have changed the signature for getitem but did not change setitem. I am not sure where it is documented if the indexing specification applies to both equally.

Each None in the selection tuple must expand the dimensions of the resulting selection by one dimension of size 1. The position of the added dimension must be the same as the position of None in the selection tuple.

I believe that the indexing specification allow `None` in a selection tuple, but the current typing does not specify this. I have changed the signature for `__getitem__` but did not change `__setitem__`. I am not sure where it is documented if the indexing specification applies to both equally.
@kgryte kgryte added Narrative Content Narrative documentation content. topic: Indexing Array indexing. labels Aug 10, 2023
@kgryte kgryte added this to the v2023 milestone Aug 10, 2023
@kgryte
Copy link
Contributor

kgryte commented Aug 10, 2023

Should this be backported to prior revisions?

@asmeurer
Copy link
Member

I guess the spec doesn't actually say anything about None as the sole index.

@saulshanabrook
Copy link
Contributor Author

I guess the spec doesn't actually say anything about None as the sole index.

Yeah it doesn't say it's allowed, so I wasn't sure if it was. Do you think the assumption was that it is allowed?

I know there are some E2E tests as well, but I wasn't sure where they live, if they contain this use case.

@asmeurer
Copy link
Member

Tests are at https://github.com/data-apis/array-api-tests/blob/master/array_api_tests/test_array_object.py#L81, but it comes down to what is implemented in indices in hypothesis https://hypothesis.readthedocs.io/en/latest/numpy.html#xps.indices. I'm not sure how well synched that is with the spec (@honno). At any rate, if there's a discrepancy between the spec and the test suite, the spec is correct.

numpy.array_api doesn't allow it, and gives the error

IndexError: self.ndim=1, but the multi-axes index only specifies 0 dimensions. If this was intentional, add a trailing ellipsis (...) which expands into as many slices (:) as necessary - this is what np.ndarray arrays implicitly do, but such flat indexing behaviour is not specified in the Array API.

which suggests that maybe this is intentional, because we decided to only allow sole-indices (non-tuple) when the index specifies every axis of the array. Actually in numpy.array_api
a[None] does work if a is 0-dimensional, which is a degenerate case. (though I'll also note that similarly if numpy.array_api and the spec disagree the spec is right).

@saulshanabrook
Copy link
Contributor Author

saulshanabrook commented Aug 14, 2023

we decided to only allow sole-indices (non-tuple) when the index specifies every axis of the array

Oh, do you mean that if you have a 2D array, for example, you can't use non-tuple indexing? I didn't catch that from the specification.

EDIT: Sorry I see you just answered that question and yes you do mean that!

@honno
Copy link
Member

honno commented Aug 15, 2023

Tests are at https://github.com/data-apis/array-api-tests/blob/master/array_api_tests/test_array_object.py#L81, but it comes down to what is implemented in indices in hypothesis https://hypothesis.readthedocs.io/en/latest/numpy.html#xps.indices. I'm not sure how well synched that is with the spec (@honno).

Yep xps.indices() generates None indices ("newaxis").

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. These changes should be backported given that None is discussed in the 2021 and 2022 revisions of the API standard. As such, the exclusion of None from the respective type definitions was an omission.

Additionally, we need to update the type signature to allow None as the sole index, given that x[None] is syntactic sugar for x[(None,)], which is explicitly noted in the specification.

I can perform the backports and updated typing in a follow-up PR.

@kgryte kgryte merged commit dea5791 into data-apis:main Sep 19, 2023
@saulshanabrook saulshanabrook deleted the saulshanabrook-patch-2 branch September 20, 2023 19:59
@saulshanabrook
Copy link
Contributor Author

Additionally, we need to update the type signature to allow None as the sole index, given that x[None] is syntactic sugar for x[(None,)], which is explicitly noted in the specification.

FWIW I don't believe x[None] translates to x[(None,)]. I think only x[None,] would:

In [1]: class A:
   ...:     def __getitem__(self, x): return x
   ...:

In [2]: A()[None]

@kgryte
Copy link
Contributor

kgryte commented Sep 20, 2023

From a typing perspective, should there be a difference? If x is zero-dimensional, providing None as the sole index should be permitted, as it follows from rules governing multi-axis indexing.

kgryte added a commit that referenced this pull request Nov 6, 2023
PR-URL: #687
Reviewed-by: Matthew Barber <quitesimplymatt@gmail.com>
Ref: #674
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Narrative Content Narrative documentation content. topic: Indexing Array indexing.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants