-
-
Notifications
You must be signed in to change notification settings - Fork 10.9k
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
Conversation
0ec1788
to
04233a6
Compare
04233a6
to
e05149a
Compare
numpy/lib/_arraysetops_impl.py
Outdated
UniqueAllResult.__module__ = "numpy" | ||
UniqueCountsResult.__module__ = "numpy" | ||
UniqueInverseResult.__module__ = "numpy" |
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 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?
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.
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.
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 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.
numpy/lib/_arraysetops_impl.pyi
Outdated
@overload | ||
def unique_all( | ||
x: _ArrayLike[_SCT], / | ||
) -> tuple[NDArray[_SCT], NDArray[intp], NDArray[intp], NDArray[intp]]: ... |
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.
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
8000
[_SCT]
inverse_indices: NDArray[intp]
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.
Sure! I added these classes to typing.
b6086c1
to
4327973
Compare
4327973
to
f7f9509
Compare
Looks good to me; In it goes. Thanks @mtsokol! |
Hi @rgommers @ngoldbaum,
This PR introduces Array API set functions. Their implementations mostly required setting
np.unique
flags correctly.Tracking issue: #25076