8000 API: Add Array API setops [Array API] by mtsokol · Pull Request #25088 · numpy/numpy · GitHub
[go: up one dir, main page]

Skip to content

API: Add Array API setops [Array API] #25088

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
Dec 5, 2023
Merged

Conversation

mtsokol
Copy link
Member
@mtsokol mtsokol commented Nov 7, 2023

Hi @rgommers @ngoldbaum,

This PR introduces Array API set functions. Their implementations mostly required setting np.unique flags correctly.

Tracking issue: #25076

@mtsokol mtsokol changed the title API: Add Array API setops API: Add Array API setops [Array API] Nov 7, 2023
@mtsokol mtsokol marked this pull request as draft November 7, 2023 14:04
@mtsokol mtsokol self-assigned this Nov 7, 2023
@mtsokol mtsokol marked this pull request as ready for review November 8, 2023 13:17
Comment on lines 388 to 390
UniqueAllResult.__module__ = "numpy"
UniqueCountsResult.__module__ = "numpy"
UniqueInverseResult.__module__ = "numpy"
Copy link
Member

Choose a reason for hiding this comment

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

I don't believe these named tuples are actually available in the main namespace (as __module__ now claims), no? This could potentially cause issue I imagine with anything relying on __module__ + __name__ for retrieving an object, e.g. does pickling even work now?

Copy link
Member Author

Choose a reason for hiding this comment

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

Good point - @rgommers Do you think that namedtuple classes of return objects for setops should be present in the main namespace? Now when I think about it they shouldn't and I would leave __module__ as numpy.lib._arraysetops.

We had similar discussion for index tricks, e.g. np.r_ which is <numpy.lib._index_tricks_impl.RClass at 0x102fd6b20>. RClass return class is in a private module only.

Copy link
Member

Choose a reason for hiding this comment

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

I don't think those should be public. But yes, pickling should work, and adding a test to confirm that would be useful. So __module__ should just not be set, then it should be automatically correct.

@overload
def unique_all(
x: _ArrayLike[_SCT], /
) -> tuple[NDArray[_SCT], NDArray[intp], NDArray[intp], NDArray[intp]]: ...
Copy link
Member

Choose a reason for hiding this comment

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

The return these here should be named tuples as well, just as is the case during runtime. It's a bit annoying, but that does mean the named tuples definitions will have to be repeated here in the .pyi file:

# NOTE: still needs a `from typing import Generic` statement

class UniqueAllResult(NamedTuple, Generic[_SCT]):
    values: NDArray[_SCT]
    indices: NDArray[intp]
    inverse_indices: NDArray[intp]
    counts: NDArray[intp]

class UniqueCountsResult(NamedTuple, Generic[_SCT]):
    values: NDArray[_SCT]
    counts: NDArray[intp]

class UniqueInverseResult(NamedTuple, Generic[_SCT]):
    values: NDArray[_SCT]
    inverse_indices: NDArray[intp]

Copy link
Member Author

Choose a reason for hiding this comment

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

Sure! I added these classes to typing.

@mtsokol mtsokol force-pushed the array-api-setops branch 2 times, most recently from b6086c1 to 4327973 Compare November 12, 2023 22:09
@mtsokol mtsokol added this to the 2.0.0 release milestone Nov 29, 2023
@ngoldbaum
Copy link
Member

Looks good to me; In it goes. Thanks @mtsokol!

@ngoldbaum ngoldbaum merged commit 0d28882 into numpy:main Dec 5, 2023
@mtsokol mtsokol deleted the array-api-setops branch December 5, 2023 16:46
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants
0