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

TYP: Fix typing in numpy2_compat.py #5101

Merged
merged 1 commit into from
Jan 25, 2025

Conversation

chrishavlin
Copy link
Contributor

Close #5100

@chrishavlin chrishavlin added the infrastructure Related to CI, versioning, websites, organizational issues, etc label Jan 23, 2025
@jzuhone jzuhone self-requested a review January 23, 2025 18:05
@neutrinoceros
Copy link
Member

This seems harmless at first sight but I don't have the faintest idea why this suddenly started to fail. Do you ?
mypy is pinned in requirements/typecheck.txt so that's not it, but clearly if type checking succeeds here, it means that something must have changed in the enviornment. Come to think of it, these comments cannot both co-exist with warn_unused_ignores and type check against both numpy 1.x and 2.x, which is also worrisome to some degree, but numpy 1 is far less type-checkable anyway so we don't loose much by not trying it.
Anyway, thanks for the ping. Unfortunately I don't understand why this is suddenly necessary, but since it clearly is, I see no reason to block it, so feel free to merge !

@chrishavlin
Copy link
Contributor Author

This seems harmless at first sight but I don't have the faintest idea why this suddenly started to fail. Do you ?

@neutrinoceros part of the reason for the ping was the hope that you'd have an idea offhand of why this was needed and I confess I didn't look into it too deeply :)

But looking again now I think I figured it out! np 2.2.2 was released last week and gets picked up in the type checking test here and has a number of typing-related changes (Full release notes). This change numpy/numpy#28178 would fix the previous attr-defined failure (and give rise to unused error code failure reported in #5100 ). But just removing the ignore (which is what I first tried locally) results in a new error:

yt/_maintenance/numpy2_compat.py:9: error: Incompatible types in assignment (expression has type "Callable[[_Buffer | _SupportsArray[dtype[Any]] | _NestedSequence[_SupportsArray[dtype[Any]]] | builtins.bool | int | float | complex | str | bytes | _NestedSequence[builtins.bool | int | float | complex | str | bytes], _Buffer | _SupportsArray[dtype[Any]] | _NestedSequence[_SupportsArray[dtype[Any]]] | builtins.bool | int | float | complex | str | bytes | _NestedSequence[builtins.bool | int | float | complex | str | bytes] | None, float, int], generic[Any] | ndarray[tuple[int, ...], dtype[generic[Any]]]]", variable has type overloaded function)  [assignment]

which... I didn't know what to do with, so added the ignore[assignment] just to get tests passing again. But looking again now, maybe setting the expected type for trapezoid is a better fix? e.g., the following diff passes mypy locally:

 import numpy as np
+from typing import Callable
 
+trapezoid: Callable
 if hasattr(np, "trapezoid"):
     # np.trapz is deprecated in numpy 2.0 in favor of np.trapezoid
     trapezoid = np.trapezoid
 else:
-    trapezoid = np.trapz  # type: ignore[assignment] # noqa: NPY201
+    trapezoid = np.trapz  # noqa: NPY201

I'm just not sure if trapezoid: Callable is sufficient?

@neutrinoceros
Copy link
Member

Ow, that makes sense ! I knew about numpy 2.2.2 but somehow missed the connection. Welp, now there's not even a residual doubt about it; we should only aim at typechecking against the latest version of numpy at this point. Thanks for the clarification !

@neutrinoceros neutrinoceros merged commit b75ef98 into yt-project:main Jan 25, 2025
13 checks passed
@neutrinoceros neutrinoceros added this to the 4.4.1 milestone Jan 26, 2025
@neutrinoceros
Copy link
Member

@meeseeksdev backport to yt-4.4.x

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
infrastructure Related to CI, versioning, websites, organizational issues, etc
Projects
None yet
Development

Successfully merging this pull request may close these issues.

mypy failure: unused error code in numpy2_compat.py
3 participants