TYP: type ma.min #26624

luxedo · 2024-06-05T19:54:32Z

Add static typing to ma.min. Part of #26404.

luxedo · 2024-06-05T21:08:46Z

I'd appreciate a little help here. This is my first time reading numpy's typing system.

How to validate typing? I did not understand why np.min has three typing declarations:

Lines 653 to 679 in 7f6ac72

    
           @overload 
        
           def amin( 
        
               a: _ArrayLike[_SCT], 
        
               axis: None = ..., 
        
               out: None = ..., 
        
               keepdims: Literal[False] = ..., 
        
               initial: _NumberLike_co = ..., 
        
               where: _ArrayLikeBool_co = ..., 
        
           ) -> _SCT: ... 
        
           @overload 
        
           def amin( 
        
               a: ArrayLike, 
        
               axis: None | _ShapeLike = ..., 
        
               out: None = ..., 
        
               keepdims: bool = ..., 
        
               initial: _NumberLike_co = ..., 
        
               where: _ArrayLikeBool_co = ..., 
        
           ) -> Any: ... 
        
           @overload 
        
           def amin( 
        
               a: ArrayLike, 
        
               axis: None | _ShapeLike = ..., 
        
               out: _ArrayType = ..., 
        
               keepdims: bool = ..., 
        
               initial: _NumberLike_co = ..., 
        
               where: _ArrayLikeBool_co = ..., 
        
           ) -> _ArrayType: ...

Is following the documentation and the examples enough?
I see there's a test dir for typing. Should I create tests as well?
I'd love some review of this PR to check if I'm in the right direction.

I'm quite confident that once I got the resources I need I can keep typing the other functions by myself

luxedo · 2024-06-18T01:50:25Z

Any updates on this?

Jacob-Stevens-Haas

So I'm working on #26081 and while not a numpy dev, I can hopefully ask and answer some questions.

np.amin has three overloads to tell the type checker three different cases
- with no axis argument, amin will return the argmin of the entire array as a python (not numpy) object, so tell the type checker that.
- With an axis argument and no out, it's impossible to tell what _ArrayLike object a is, so tell type checker Any
- With an argument out, we know that the returned variable will be out, so the type checker knows the return type matches out
  These are probably also relevant for masked arrays.
I don't think you need more documentation, because there's no new types, other than an entry in doc/release/upcoming_changes/26624.improvement.rst.
Yes, most likely. A file numpy/typing/tests/data/pass/masked.py and likewise .../fail/masked.py that will fill up with type checks as #26404 is implemented. This is a good place to test-drive assumptions you might have about how your type annotations work as you develop the PR (e.g. the comment on axis and obj return types)

Jacob-Stevens-Haas · 2024-07-01T17:39:25Z

numpy/ma/core.pyi

+    axis: None | _ShapeLike = ...,
+    out: None = ...,
+    fill_value: None | _ScalarLike_co = ...,
+    keepdims: None | bool = ...,
+) -> _SCT: ...


if axis is _ShapeLike and not None, the return type will be the same type as obj, not _SCT.

luxedo · 2024-07-04T15:03:31Z

@Jacob-Stevens-Haas thank you for the feedback. I'll be working on this next week.

charris · 2024-08-30T15:38:26Z

The test failure can be ignored.

charris · 2024-08-30T15:38:42Z

Need rebase.

luxedo · 2024-09-01T03:35:31Z

Rebased and upated with @Jacob-Stevens-Haas hints. Also added passing typing tests.

I couldn't understand how to make failing tests. When calling spin mypy somehow it kept throwing me old errors even after deleting the test file.

Jacob-Stevens-Haas

The PR is great, a few comments about tests (with a grain of salt - I'm learning too):

I couldn't understand how to make failing tests.

Fail tests are written with arguments that fail type checking, with a comment afterwards for the expected type checking error (mostly either "no overload variant" or "argument has incompatible type" e.g.

numpy/numpy/typing/tests/data/fail/fromnumeric.pyi

Lines 126 to 130 in d52816a

    
           np.amin(a, axis=1.0)  # E: No overload variant 
        
           np.amin(a, keepdims=1.0)  # E: No overload variant 
        
           np.amin(a, out=1.0)  # E: No overload variant 
        
           np.amin(a, initial=[1.0])  # E: No overload variant 
        
           np.amin(a, where=[1.0])  # E: incompatible type

Edit: But thinking about it more, there's no illegal combination I can think of worth testing. E.g. the overloads allow np.min(1.0) and np.min(arr, axis=0). If they prevented something like np.ma.min(1.0, axis=0), that would be a good fail test. I don't think there are any such holes in the overloads for this PR though, so fail tests may not be necessary

There's also reveal tests that verify the type that gets returned with assert_type. My mental comparison is that pass/fail tests check arguments, reveal tests check return value. In this case, it might be helpful to check the returned type is not Any in overloads 1 and 3.

When calling spin mypy somehow it kept throwing me old errors even after deleting the test file.

Did you try running $ git clean -xdf?

Jacob-Stevens-Haas · 2024-09-10T21:33:19Z

numpy/typing/tests/data/pass/ma.py

-m : np.ma.MaskedArray[Any, np.dtype[np.float64]] = np.ma.masked_array([1.5, 2, 3], mask=[True, False, True])
+m: np.ma.MaskedArray[Any, np.dtype[np.float64]] = np.ma.masked_array([1.5, 2, 3], mask=[True, False, True])

+A = np.array(True, ndmin=2, dtype=bool)


I'm no expert on which tests to write for pass/, but this may be more than is required? IIUC, the PR doesn't make a distinction between bool, np.uint8, np.float32, np.uint8, so does the distinction between A/B and C/D really test anything? (happy to change my mind though!)

Jacob-Stevens-Haas · 2024-09-10T21:33:48Z

numpy/typing/tests/data/pass/ma.py

+A.setflags(write=False)
+B.setflags(write=False)
+C.setflags(write=False)


I see that setflags is called in fromnumeric.py, but does it do anything important for the test?

Jacob-Stevens-Haas · 2024-09-10T21:34:39Z

numpy/typing/tests/data/pass/ma.py

+np.ma.min(A, axis=0)
+np.ma.min(B, axis=0)


On the other hand, axis being None, an int, or _ShapeLike does make a difference. It might make sense to also run this with axis=(0,)

luxedo · 2024-09-26T02:23:54Z

@Jacob-Stevens-Haas I updated the PR with your suggestions

Jacob-Stevens-Haas

Thanks for updating this! Can you add a reveal test to check the return values? I.e. numpy/typing/tests/data/reveal/ma.pyi like:

import numpy as np

arr: np.ma.MaskedArray[Any, np.dtype[np.float64]] = np.ma.masked_array([[1.0, 2.0]], mask=[True, False])
result = np.ma.min(arr, axis=None)
assert_type(result, np.float64)

tgt: np.ma.MaskedArray[Any, np.dtype[np.float64]] = np.ma.empty(shape=())
result = np.ma.min(arr, axis=None, out=tgt)
assert_type(result, np.ma.MaskedArray)

This verifies that the result type changes with the out parameter. Since reveal tests even need to be executable, so you can remove the assignment for readability.

Co-Authored-By: Jakob Stevens Haas <[email protected]>

luxedo · 2024-09-30T03:14:42Z

Both my numpy's and scipy's builds are broken since last week :(

I pushed this test and it failed. I'll try to make spin mypy work and then I fix the typing.

Jacob-Stevens-Haas · 2024-10-09T17:32:47Z

Hmm, this is curious. keepdims defaults to _NoValue, not None or False, even though the default behavior is False. Thus the static typing sees the failing test as overload 2 instead of overload 1.

If the default value is passed, then keepdims will not be passed through to the min method of sub-classes of ndarray, however any non-default value will be. If the sub-class’ method does not implement keepdims any exceptions will be raised.

In other words, _NoValueType exists in order to allow subclasses to break Liskov Substitution, which means might be a problem in and of itself. It looks like _NoValueType was first discussed as a sort of third option for an argument and has come up again in a few PRs and issues 1, 2, and 3.

The long and short of this is that you can probably include _NoValueType in overload 1, e.g. keep_dims: Literal[False] | _NoValueType. But it would also be good to check whether we really need to allow _NoValue as the default or we can just default to false.

jorenham

Thanks for this!

I left a couple of minor suggestions, but in general, I like the way this looks 🙁.

jorenham · 2024-11-04T17:07:05Z

numpy/ma/core.pyi

-def min(obj, axis=..., out=..., fill_value=..., keepdims=...): ...
+@overload
+def min(
+    obj: MaskedArray[_ShapeLike, _SCT] | _ArrayLike[_SCT],


The shape type is bound to tuple[int, ...], to which _ShapeLike isn't assignable (i.e. it's a broader type).

Additionally, the second type parameter of MaskedArray is bound to np.dtype, but _SCT is bound to np.generic.

Luckily this is easy to fix in this case, because MaskedArray is assignable to _ArrayLike (for a specific scalar type _SCT). So that means we simply write it as:

Suggested change

obj: MaskedArray[_ShapeLike, _SCT] | _ArrayLike[_SCT],

obj: _ArrayLike[_SCT],

jorenham · 2024-11-04T17:26:45Z

numpy/ma/core.pyi

+    axis: None = ...,
+    out: None = ...,
+    fill_value: None | _ScalarLike_co = ...,
+    keepdims: Literal[False] = ...,


I believe this was discussed earlier, but keepdims uses a default sentinel, and it's impossible to type it.
However, that's not a problem at all. Because from a typing perspective, not passing keepdims is indistinguishable from keepdims=False. So we can just pretend that Literal[False] is the default.
So in other words; this overload is perfectly fine as-is.

jorenham · 2024-11-04T17:33:41Z

numpy/ma/core.pyi

+    out: None = ...,
+    fill_value: None | _ScalarLike_co = ...,
+    keepdims: Literal[False] = ...,
+) -> _SCT: ...


If the array has an object_ as scalar-type, then this implies that an instance of np.object_ is returned. However, those don't exist. And because we don't know what the actual type is "within" that object_, the best we could do is to return an object, and let the user cast it to the specific type.

But having said that, I don't think that it's really a priority at the moment. So I personally wouldn't mind if you'd label the returned object_ as a "proxy type", and keep it the way it is 🤷🏻.
_{Plus, a little bird told me that object_ might become a generic type in the near future 😏.}

jorenham · 2024-11-04T17:43:39Z

numpy/ma/core.pyi

+
+@overload
+def min(
+    obj: MaskedArray[_ShapeLike, _SCT] | _ArrayLike[_SCT],


Suggested change

obj: MaskedArray[_ShapeLike, _SCT] | _ArrayLike[_SCT],

obj: _ArrayLike[_SCT],

jorenham · 2024-11-04T17:50:03Z

numpy/ma/core.pyi

+    out: None = ...,
+    fill_value: None | _ScalarLike_co = ...,
+    keepdims: None | bool = ...,
+) -> Any: ...


I realize that this is in line with the current annotations of numpy.min, but we could also narrow this down do _SCT | MaskedArray[tuple[int, ...], np.dtype[_SCT]] while we're at it 🤷🏻.

_{Like I mentioned before, this technically this isn't correct in the case of object_ (and StringDType as well, BTW). But because this is already an improvement in general, I think it's not too big of a problem if we just ignore this for now.}

jorenham · 2024-11-04T17:58:53Z

numpy/typing/tests/data/reveal/ma.pyi

+
+from typing_extensions import assert_type
+
+arr: np.ma.MaskedArray[Any, np.dtype[np.float64]] = np.ma.masked_array([[1.0, 2.0]], mask=[True, False])


there's no need to do the actual function call within a .pyi stub, especially since the return type of np.ma.masked_array was already verified in the "pass" tests.

Suggested change

arr: np.ma.MaskedArray[Any, np.dtype[np.float64]] = np.ma.masked_array([[1.0, 2.0]], mask=[True, False])

arr: np.ma.MaskedArray[Any, np.dtype[np.float64]]

jorenham · 2024-11-04T18:05:11Z

numpy/typing/tests/data/reveal/ma.pyi

+result = np.ma.min(arr, axis=None)
+assert_type(result, np.float64)


by inlining the result, you avoid changing the type result later on, which technically isn't valid (mypy allowing this is a bug IMHO).

Suggested change

result = np.ma.min(arr, axis=None)

assert_type(result, np.float64)

assert_type(np.ma.min(arr, axis=None), np.float64)

jorenham · 2024-11-04T18:11:18Z

numpy/typing/tests/data/reveal/ma.pyi

+tgt: np.ma.MaskedArray[Any, np.dtype[np.float64]] = np.ma.empty(shape=())
+result = np.ma.min(arr, axis=None, out=tgt)
+assert_type(result, np.ma.MaskedArray)


The type of tgt is equivalent to that of arr, so there's no need for it (so it doesn't matter that at runtime this wouldn't make a lot of sense).

Additionally, np.ma.MaskedArray should have type arguments, as its type parameters have no defaults (PEP 696).

Suggested change

tgt: np.ma.MaskedArray[Any, np.dtype[np.float64]] = np.ma.empty(shape=())

result = np.ma.min(arr, axis=None, out=tgt)

assert_type(result, np.ma.MaskedArray)

assert_type(np.ma.min(arr, axis=None, out=arr), np.ma.MaskedArray[Any, np.dtype[np.float64]])

jorenham · 2024-11-04T18:13:01Z

numpy/ma/core.pyi

    zeros_like as zeros_like,
    angle as angle
 )
+from numpy._typing import _ArrayLike, _ScalarLike_co, _ShapeLike, overload


It's probably safer to import overload directly from typing instead.

jorenham · 2024-11-04T18:14:28Z

numpy/ma/core.pyi


 _ShapeType_co = TypeVar("_ShapeType_co", bound=tuple[int, ...], covariant=True)
 _DType_co = TypeVar("_DType_co", bound=dtype[Any], covariant=True)
+_ArrayType = TypeVar("_ArrayType", bound="MaskedArray[Any, Any]")


There's no need for stringified annotations within .pyi stubs (and disallowed by e.g. flake8-pyi and several type-checkers)

jorenham · 2024-11-04T18:24:59Z

I see that there's also a (tiny) merge conflict.

jorenham · 2025-03-28T15:16:38Z

This has been superseded by #28593, so I'm going to close this.

github-actions bot added the 41 - Static typing label Jun 5, 2024

luxedo marked this pull request as ready for review June 9, 2024 23:28

Jacob-Stevens-Haas reviewed Jul 1, 2024

View reviewed changes

TYP: type ma.min

5535bd0

luxedo force-pushed the enh/26404 branch from ddabc4a to 724c2f5 Compare September 1, 2024 03:10

TYP: add ma.main typing tests

d52816a

luxedo force-pushed the enh/26404 branch from 724c2f5 to d52816a Compare September 1, 2024 03:30

luxedo mentioned this pull request Sep 2, 2024

ENH: add type annotations to numpy.ma #26404

Open

Jacob-Stevens-Haas reviewed Sep 11, 2024

View reviewed changes

Jacob-Stevens-Haas suggested changes Sep 29, 2024

View reviewed changes

luxedo and others added 2 commits September 30, 2024 00:10

TST: fixing ma.min typing tests

6e74f3b

Co-Authored-By: Jakob Stevens Haas <[email protected]>

TST: add ma.min reveal typing tests

bd55881

Co-Authored-By: Jakob Stevens Haas <[email protected]>

luxedo force-pushed the enh/26404 branch from 7444dbe to bd55881 Compare September 30, 2024 03:10

jorenham requested changes Nov 4, 2024

View reviewed changes

MarcoGorelli mentioned this pull request Mar 27, 2025

TYP: Type numpy.ma.min #28593

Merged

jorenham closed this Mar 28, 2025

	np.amin(a, axis=1.0) # E: No overload variant
	np.amin(a, keepdims=1.0) # E: No overload variant
	np.amin(a, out=1.0) # E: No overload variant
	np.amin(a, initial=[1.0]) # E: No overload variant
	np.amin(a, where=[1.0]) # E: incompatible type

	obj: MaskedArray[_ShapeLike, _SCT] \| _ArrayLike[_SCT],
	obj: _ArrayLike[_SCT],


		from typing_extensions import assert_type

		arr: np.ma.MaskedArray[Any, np.dtype[np.float64]] = np.ma.masked_array([[1.0, 2.0]], mask=[True, False])

		result = np.ma.min(arr, axis=None)
		assert_type(result, np.float64)

	result = np.ma.min(arr, axis=None)
	assert_type(result, np.float64)
	assert_type(np.ma.min(arr, axis=None), np.float64)

		np.ma.min(A, axis=0)
		np.ma.min(B, axis=0)

Uh oh!

TYP: type ma.min #26624

TYP: type ma.min #26624

Uh oh!

Conversation

luxedo commented Jun 5, 2024

Uh oh!

luxedo commented Jun 5, 2024

Uh oh!

luxedo commented Jun 18, 2024

Uh oh!

Jacob-Stevens-Haas left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

luxedo commented Jul 4, 2024

Uh oh!

charris commented Aug 30, 2024

Uh oh!

charris commented Aug 30, 2024

Uh oh!

luxedo commented Sep 1, 2024

Uh oh!

Jacob-Stevens-Haas left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Jacob-Stevens-Haas Sep 10, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

luxedo commented Sep 26, 2024

Uh oh!

Jacob-Stevens-Haas left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

luxedo commented Sep 30, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Jacob-Stevens-Haas commented Oct 9, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jorenham left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jorenham commented Nov 4, 2024

Uh oh!

jorenham commented Mar 28, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Jacob-Stevens-Haas left a comment •

edited

Loading

Jacob-Stevens-Haas Sep 10, 2024 •

edited

Loading

Jacob-Stevens-Haas left a comment •

edited

Loading

luxedo commented Sep 30, 2024 •

edited

Loading

Jacob-Stevens-Haas commented Oct 9, 2024 •

edited

Loading