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

Replace int with SupportsIndex in indexing methods hints #766

Open
wants to merge 4 commits into
base: main
Choose a base branch
from

Conversation

honno
Copy link
Member

@honno honno commented Mar 21, 2024

Resolves #383

@rgommers
Copy link
Member

Thanks @honno.

Adding a quick clarifying example:

>>> import numpy as np
>>> import torch
>>> 
>>> x = np.arange(1, 4)
>>> y = torch.arange(1, 4)
>>> 
>>> class Ix:
...     def __index__(self):
...         return 1
... 
>>> x[Ix()]
2
>>> y[Ix()]
tensor(2)

@honno honno requested a review from rgommers March 22, 2024 08:31
Might fix rendering/warning issue
@rgommers rgommers added the API extension Adds new functions or objects to the API. label Mar 25, 2024
@rgommers
Copy link
Member

@honno would you be able to open a PR with a test for this to array-api-tests? Doesn't have to be merged before this will be merged, but that will help smoke out if any known/tested library does not yet support this feature.

index key.


.. note::
``key`` can only be an array if it is valid for boolean array indexing, or supports ``__index__()``.
Copy link
Member

Choose a reason for hiding this comment

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

Presumably somewhere else we say it should support __index__ if and only if it is a 0-D integer array. Maybe it would be clearer to just say that directly here.

Copy link
Member Author

Choose a reason for hiding this comment

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

Yep that'd help, let me know on the wording from my latest commit.

@honno
Copy link
Member Author

honno commented Mar 27, 2024

@honno would you be able to open a PR with a test for this to array-api-tests? Doesn't have to be merged before this will be merged, but that will help smoke out if any known/tested library does not yet support this feature.

Good shout, I opened data-apis/array-api-tests#247 to check this all out. From the looks of it:

  • NumPy (+ array_api_strict) and PyTorch supports this behaviour.
  • JAX doesn't support, has a general note to open a feature requests on indexing modes (... that's prob refering to advance integer indexing).
  • cupy seems to only support advance integer indexing and doesn't play nice with 0-D integer arrays as indexes.
  • Dask at least accepts 0-D integer input, but array_api_compat.dask not playing nice with the test suite so I'll have to triage that and explore further.

@asmeurer
Copy link
Member

but array_api_compat.dask not playing nice with the test suite so I'll have to triage that and explore further.

Let me know what you find out. We do run it on CI with some skips and xfails, and also the max-examples is set to 5. I haven't looked at the Dask xfails too closely, and obviously if we can remove any of those that would be great. CC @lithomas1

@kgryte
Copy link
Contributor

kgryte commented Apr 18, 2024

@honno Were you able to triage the Dask issues with the test suite?

@honno
Copy link
Member Author

honno commented Apr 22, 2024

Got Dask working1—the latest release at least doesn't support indexables for both get and set items right now (TypeError for an internal inequality check which assumes indexes as ints).

Footnotes

  1. Turns out I should of been using array_api_compat.dask.array heh, although there's another unrelated issue Dask has with one of our utilities I'll have to explore.

@asmeurer
Copy link
Member

By the way, for implementers, the generally correct behavior is to operator.index() to normalize index objects that aren't one of the other supported index types like Ellipsis, slice, or array. My guess is that dask.array isn't doing that.

@kgryte
Copy link
Contributor

kgryte commented Sep 19, 2024

@honno Is there anything more that we need to do with this PR?

@kgryte kgryte added topic: Indexing Array indexing. Needs Review Pull request which needs review. labels Sep 19, 2024
@honno
Copy link
Member Author

honno commented Oct 13, 2024

@honno Is there anything more that we need to do with this PR?

PR I think I'm happy with, just be mindful that last I checked in March it was only NumPy and PyTorch that supported "indexables", whereas JAX/CuPy/Dask didn't, so they'd need updating to support this.

Example of what I mean by an indexable:

class AwkwardIndexable:
    def __init__(self, value: int):
        self._value = value

    def __int__(self):
        raise TypeError("__int__() should not be called")

    def __index__(self):
        return self._value

@kgryte kgryte added the Needs Discussion Needs further discussion. label Oct 31, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
API extension Adds new functions or objects to the API. Needs Discussion Needs further discussion. Needs Review Pull request which needs review. topic: Indexing Array indexing.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Specify that dunder get/set item accepts SupportsIndex
4 participants