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

ENH: Add annotations for np.lib.arrayterator #18545

Merged
merged 4 commits into from
Mar 12, 2021
Merged

Conversation

BvB93
Copy link
Member

@BvB93 BvB93 commented Mar 4, 2021

This PR adds annotations for the Arrayterator class in np.lib.arrayterator.

Two things of note:

  • The __getattr__ method of Arrayterator wraps around the underlying ndarrays attributes.
    To simulate this behavior the former has been typed such that it subclasses the latter
    (even though, strictly speaking, this isn't true at runtime).
  • The dtype parameter was missing from the Arrayterator.__array__ method. It has now been added.

Copy link
Member

@rgommers rgommers left a comment

Choose a reason for hiding this comment

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

Thanks @BvB93. Inheriting from ndarray in the stub seems reasonable. The changes to the class itself need a tweak.

@@ -124,14 +124,17 @@ def __getitem__(self, index):
out.stop[i] = min(stop, out.stop[i])
return out

def __array__(self):
def __array__(self, dtype=None):
Copy link
Member

Choose a reason for hiding this comment

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

Looks to me like dtype should be a positional argument:

>>> x = np.arange(5)
>>> x.__array__(np.float32)
array([0., 1., 2., 3., 4.], dtype=float32)
>>> x.__array__(dtype=np.float32)
Traceback (most recent call last):
  File "<ipython-input-43-6adb0d04469e>", line 1, in <module>
    x.__array__(dtype=np.float32)
TypeError: __array__() takes no keyword arguments

Copy link
Member Author

Choose a reason for hiding this comment

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

Based on some trial and error it seems that the dtype parameter must have a default value but can otherwise be either position-only or positional-or-keyword (ndarray being an example of the former).

Secondly, I'm not convinced anymore that this change is even necessary in the first place,
as I was initially under the (incorrect) impression that a dtype parameter would be required here
if one wants to use it with the likes of, e.g., np.array(x, dtype=<insert dtype different from x's dtype>).

I think it might be best to revert this change as well and then make the npt._SupportsArray protocol a bit more flexible, such that it can accomodate __array__ without the dtype parameter.

Copy link
Member

Choose a reason for hiding this comment

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

Yes, adding the dtype keyword is not needed, reverting sounds good.

numpy/lib/arrayterator.py Outdated Show resolved Hide resolved
Bas van Beek added 4 commits March 12, 2021 16:56
Implementations of `__array__` do not require the `dtype` parameter
The likes of `np.array(..., dtype=dtype)` will always pass `np.dtype` instances to `__array__(self, dtype=...)`, so that latter should not have to take arbitrary dtype-like objects but only actual `np.dtype` instances.
@rgommers
Copy link
Member

rebased to avoid the lint-check early CI termination.

Copy link
Member

@rgommers rgommers left a comment

Choose a reason for hiding this comment

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

This LGTM now, so let's get it in. Thanks @BvB93

@rgommers rgommers merged commit cb7c292 into numpy:main Mar 12, 2021
@@ -14,4 +14,4 @@
reveal_type(a[...]) # E: numpy.ndarray[Any, numpy.dtype[numpy.str_]]
reveal_type(a[:]) # E: numpy.ndarray[Any, numpy.dtype[numpy.str_]]
reveal_type(a.__array__()) # E: numpy.ndarray[Any, numpy.dtype[numpy.str_]]
reveal_type(a.__array__(np.float64)) # E: numpy.ndarray[Any, numpy.dtype[Any]]
reveal_type(a.__array__(np.dtype(np.float64))) # E: numpy.ndarray[Any, numpy.dtype[{float64}]]
Copy link
Member

Choose a reason for hiding this comment

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

One follow-up question: numpy.dtype[{float64}] is a little verbose. How hard would it be to make numpy.ndarray[Any, numpy.float64] work?

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.

None yet

2 participants