ENH: Add annotations to the last 8 functions in numpy.core.fromnumeric #16729

BvB93 · 2020-07-02T12:52:15Z

This pull requests adds annotations to 8 functions numpy.core.fromnumeric,
thus adding annotations to the last untyped functions from aforementioned module.

The functions in question are:

prod()
cumprod()
ndim()
size()
around()
mean()
std()
var()

The 8 respective functions, all located in `np.core.fromnumeric`, consist of: * `prod` * `cumprod` * `ndim` * `size` * `around` * `mean` * `std` * `var`

eric-wieser · 2020-07-02T13:04:10Z

numpy/tests/typing/fail/fromnumeric.py

+np.prod(a, axis=1.0)  # E: No overload variant of "prod" matches argument type
+np.prod(a, out=False)  # E: No overload variant of "prod" matches argument type
+np.prod(a, keepdims=1.0)  # E: No overload variant of "prod" matches argument type
+np.prod(a, initial=int)  # E: No overload variant of "prod" matches argument type


This is legal for suitable a:

>>> a = np.array([], dtype=object) >>> np.prod(a, initial=int) int

Or for an almost plausible use:

>>> from ctypes import c_uint8 >>> shape = np.array([1, 2, 3]) >>> np.prod(shape.astype(object), initial=c_uint8) # a ctypes array type numpy.core.fromnumeric.c_ubyte_Array_1_Array_2_Array_3

Incidentally, this also proves that the return type of np.prod can be anything, not just number.

Incidentally, this also proves that the return type of np.prod can be anything, not just number.

If I'm not mistaken this is limited to, the notoriously difficult to type, object arrays.
If so I feel it'd be better to leave things as they are for now, especially since this appears to be a very specific (seemingly undocumented?) situation.

The alternative would be to set number and the return type to Any,
which is practically useless from a typing standpoint.

especially since this appears to be a very specific (seemingly undocumented?) situation.

How so? initial is documented as a "scalar", not as a number.

Perhaps the compromise is to stick # TODO: This is actually legal or similar by each test case that currently expects and results in an error, but should not.

How so? initial is documented as a "scalar", not as a number.

That's actually a good point; I more or less conflated scalar with numerical scalar here.

Perhaps the compromise is to stick # TODO: This is actually legal or similar by each test case that currently expects and results in an error, but should not.

I agree, I'll push an update in a bit.

As for the future:

What I've gathered is, in the case of object arrays, that np.prod() simply falls back to the .__mult__() method of the passed arrays individual elements and hence all that's required of initial is that its type is compatible with aforementioned elements.
For object arrays we may or may not be able to easily express this with a __mult__ Protocol once ndarray is generic over its dtype, though this will depend on the exact implementation details.

Perhaps the compromise is to stick # TODO: This is actually legal or similar by each test case that currently expects and results in an error, but should not.

Added in 3bc51b8.

For object arrays we may or may not be able to easily express this with a __mult__ Protocol once ndarray is generic over its dtype, though this will depend on the exact implementation details.

I wouldn't personally bother. Simply having prod(ndarray[int], initial: int) -> int, prod(ndarray[object], initial: object) -> object etc is all that's really needed. The details of exactly what the object loops of ufuncs do and do not support is not really interesting.

numpy/__init__.pyi

eric-wieser · 2020-07-02T15:23:52Z

numpy/__init__.pyi

+@overload
+def var(
+    a: ArrayLike,
+    axis: None = ...,
+    dtype: DtypeLike = ...,
+    out: Optional[ndarray] = ...,
+    ddof: int = ...,
+    keepdims: Literal[False] = ...,
+) -> number: ...


number isn't correct when out is specified:

>>> np.var([1], out=np.zeros(())) array(0.)

This applies for most but not all functions in this patch with out.

This applies for most but not all functions in this patch with out.

Considering the widespread nature of this issue I'd propose to save it for a future maintenance pull request and fix it all at once (as there are already quite a few older annotated functions with out parameter).

In the meantime then, it would probably be a good idea to annotate these with Union[number, ndarray] - weak annotations are better than incorrect ones.

As it turns out is fairly easy way to fix this: out: Optional[ndarray] must be changed into out: None when a number is returned.

I have noticed some inconsistent behavior when using out in combination with passing a number though,
as in such case another number is returned rather than a ndarray.
Builtin scalars, builtin sequences and ndarrays do all return an ndarray (see example below).

In [1]: import numpy as np In [2]: np.__version__ Out[2]: '1.20.0.dev0+62f26cf' In [3]: np.var(np.int64(1), out=np.zeros(())) Out[3]: 0.0 In [4]: np.var(1, out=np.zeros(())) Out[4]: array(0.) In [5]: np.var([1], out=np.zeros(())) Out[5]: array(0.) In [6]: np.var(np.ones(1), out=np.zeros(())) Out[6]: array(0.)

That is arguably a bug in np.var, when out is given, it must be the return value.

number isn't correct when out is specified:

Fixed as of 5a42440.

That is arguably a bug in np.var, when out is given, it must be the return value.

I'll double check which (currently annotated) functions are affected by this this and create an issue afterwards (see #16734).

Clarified that the comment holds for all `np.ufunc.reduce()` wrappers

Return `out` if `out != None`

anirudh2290 · 2020-07-16T01:23:27Z

numpy/__init__.pyi

+    keepdims: Literal[False] = ...,
+    initial: _NumberLike = ...,
+    where: _ArrayLikeBool = ...,
+) -> number: ...


I am assuming we are allowing number subtypes for one but not the other why is that ?
In other words, why is there a difference in return types of the above two overloads ?

The key difference is that the first one is a TypeVar while the latter is not.

To illustrate, in the first overload an arbitrary number subclass (denoted by _Number)
is provided as input and a value of the same type is returned.

In the second overload we know that an instance of np.int_ is returned,
but there is a catch: np.int_ is a platform-dependent alias for one of the number subclasses,
which is not something we've been able to easily express as of yet.

numpy/numpy/__init__.pyi

Lines 511 to 517 in 92665ab

# TODO(alan): Platform dependent types

# longcomplex, longdouble, longfloat

# bytes, short, intc, intp, longlong

# half, single, double, longdouble

# uint_, int_, float_, complex_

# float128, complex256

# float96

As a workaround the latter case is now simply annotated as returning a number instance and
while correct, this is obviously not as specific as it could be.

okay got it, if my understanding is correct, the functionality is exactly the same if we use number of _Number here (second overload), but it is something we want to change in the future ?

..., but it is something we want to change in the future ?

~~Correct, once we've dealt with these platform-dependent generic subclasses.~~

Oh hang on, I'm actually confusing the issues pertaining int_ and number here, sorry.

The problem with returningnumber is more straightforward.
As ArrayLike is not yet Generic with respect to the data type we don't have a way yet to correlate the input and output types.
In the future we should be able to use expressions such as the one below (or at least something similar):

>>> from typing import TypeVar >>> import numpy as np >>> from numpy.typing import ArrayLike >>> T = TypeVar('T', bound=np.number) >>> def func(a: ArrayLike[T]) -> T: ... pass

If you're interested, see #16759 for more details.

numpy/__init__.pyi

anirudh2290

LGTM thanks ! IMO, it would be nice to have np.prod(...,dtype=None) in tests.

BvB93 · 2020-07-16T18:29:52Z

LGTM thanks ! IMO, it would be nice to have np.prod(...,dtype=None) in tests.

Done in 5c2dd87.
FYI, #16622 also expands the current dtype tests a bit and includes a new dtype(None) test.

BvB93 · 2020-08-06T10:21:08Z

Are there any more remarks or comments on this pull request?
If not, then let's merge.

numpy/__init__.pyi

Co-authored-by: Eric Wieser <wieser.eric@gmail.com>

BvB93 · 2020-08-06T11:03:27Z

circleci build failure seems to be unrelated.

mattip · 2020-08-06T11:14:30Z

Unfortunately, circleci does not merge-from-master before building. You can ignore the error, merge from master, or rebase off master.

BvB93 · 2020-08-06T11:17:22Z

You can ignore the error, merge from master, or rebase off master.

I'd say let's leave it as it is for now in order to not trigger any more (arguably) redundant CI runs.

mattip · 2020-08-24T11:27:44Z

Thanks @BvB93

Added annotations for 8 new functions

8a95c66

The 8 respective functions, all located in `np.core.fromnumeric`, consist of: * `prod` * `cumprod` * `ndim` * `size` * `around` * `mean` * `std` * `var`

BvB93 added 01 - Enhancement Static typing labels Jul 2, 2020

BvB93 requested a review from person142 July 2, 2020 12:52

eric-wieser reviewed Jul 2, 2020

View reviewed changes

Added a note about np.prod() and object arrays

3bc51b8

eric-wieser reviewed Jul 2, 2020

View reviewed changes

numpy/__init__.pyi Outdated Show resolved Hide resolved

eric-wieser reviewed Jul 2, 2020

View reviewed changes

numpy/__init__.pyi Outdated Show resolved Hide resolved

eric-wieser reviewed Jul 2, 2020

View reviewed changes

Bas van Beek added 2 commits July 2, 2020 17:39

addressed numpy#16729 (comment) and numpy#16729 (comment)

62f26cf

Clarified that the comment holds for all `np.ufunc.reduce()` wrappers

Addressed numpy#16729 (comment)

5a42440

Return `out` if `out != None`

BvB93 mentioned this pull request Jul 2, 2020

Scalars are returned when passing 0D arrays to out #16734

Closed

anirudh2290 reviewed Jul 16, 2020

View reviewed changes

Added two np.prod(..., dtype=None) tests

5c2dd87

anirudh2290 approved these changes Jul 16, 2020

View reviewed changes

eric-wieser reviewed Aug 6, 2020

View reviewed changes

numpy/__init__.pyi Outdated Show resolved Hide resolved

STY: Fixed a typo

c2e27c2

Co-authored-by: Eric Wieser <wieser.eric@gmail.com>

mattip merged commit 6989899 into numpy:master Aug 24, 2020

BvB93 deleted the from-numeric branch August 24, 2020 16:13

BvB93 mentioned this pull request Aug 28, 2020

Big list of functions in the top-level namespace missing types #16546

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

ENH: Add annotations to the last 8 functions in numpy.core.fromnumeric #16729

ENH: Add annotations to the last 8 functions in numpy.core.fromnumeric #16729

BvB93 commented Jul 2, 2020

eric-wieser Jul 2, 2020 •

edited

eric-wieser Jul 2, 2020

BvB93 Jul 2, 2020

eric-wieser Jul 2, 2020 •

edited

BvB93 Jul 2, 2020 •

edited

BvB93 Jul 2, 2020

eric-wieser Jul 2, 2020 •

edited

eric-wieser Jul 2, 2020

eric-wieser Jul 2, 2020 •

edited

BvB93 Jul 2, 2020 •

edited

eric-wieser Jul 2, 2020

BvB93 Jul 2, 2020

seberg Jul 2, 2020

BvB93 Jul 2, 2020

BvB93 Jul 2, 2020 •

edited

anirudh2290 Jul 16, 2020

BvB93 Jul 16, 2020

anirudh2290 Jul 16, 2020

BvB93 Jul 16, 2020 •

edited

BvB93 Jul 16, 2020

anirudh2290 left a comment

BvB93 commented Jul 16, 2020

BvB93 commented Aug 6, 2020

BvB93 commented Aug 6, 2020

mattip commented Aug 6, 2020

BvB93 commented Aug 6, 2020

mattip commented Aug 24, 2020

	# TODO(alan): Platform dependent types
	# longcomplex, longdouble, longfloat
	# bytes, short, intc, intp, longlong
	# half, single, double, longdouble
	# uint_, int_, float_, complex_
	# float128, complex256
	# float96

ENH: Add annotations to the last 8 functions in numpy.core.fromnumeric #16729

ENH: Add annotations to the last 8 functions in numpy.core.fromnumeric #16729

Conversation

BvB93 commented Jul 2, 2020

eric-wieser Jul 2, 2020 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

eric-wieser Jul 2, 2020 • edited

Choose a reason for hiding this comment

BvB93 Jul 2, 2020 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

eric-wieser Jul 2, 2020 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

eric-wieser Jul 2, 2020 • edited

Choose a reason for hiding this comment

BvB93 Jul 2, 2020 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

BvB93 Jul 2, 2020 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

BvB93 Jul 16, 2020 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

anirudh2290 left a comment

Choose a reason for hiding this comment

BvB93 commented Jul 16, 2020

BvB93 commented Aug 6, 2020

BvB93 commented Aug 6, 2020

mattip commented Aug 6, 2020

BvB93 commented Aug 6, 2020

mattip commented Aug 24, 2020

eric-wieser Jul 2, 2020 •

edited

eric-wieser Jul 2, 2020 •

edited

BvB93 Jul 2, 2020 •

edited

eric-wieser Jul 2, 2020 •

edited

eric-wieser Jul 2, 2020 •

edited

BvB93 Jul 2, 2020 •

edited

BvB93 Jul 2, 2020 •

edited

BvB93 Jul 16, 2020 •

edited