-
Notifications
You must be signed in to change notification settings - Fork 302
Refactor: replace np.ndarray with npt.NDArray in type hints #5346
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
base: main
Are you sure you want to change the base?
Conversation
|
Hi! Welcome, and thanks for opening this pull request. We have some guidelines for new pull requests, and soon you'll hear back about the results of our tests and continuous integration checks. Thank you for your contribution! |
Fix Mypy Type ErrorsObjective: Resolve type errors reported by mypy, specifically "Bad number of arguments for type alias" and "Incompatible type" errors. ChangesA. Fix
|
chrishavlin
left a comment
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.
here's a quick note. also, i will not have time to review in detail for a few days.
yt/loaders.py
Outdated
| from urllib.parse import urlsplit | ||
|
|
||
| import numpy as np | ||
| import numpy.typing as nptype |
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.
let's use import numpy.typing as npt everywhere. You've got a mix of imports across files, please just stick to import numpy.typing as npt.
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.
npt was already in use as an alias at some files , so I thought nptype would be better choice to avoid any issues .
Also can you help me add label to the pr , I don't know how to add it.
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.
can you point out a spot where it's being used as an alias? I don't see it in this file or the other couple I just checked. npt is preferred. i'll add the label when I get to doing a full review.
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 alias nptype was chosen to avoid conflicts with numpy.testing, which is often imported as npt.
it's true that we use npt for both namespaces, however I don't think they are ever imported in the same module (we don't have type hints in tests, and we shouldn't be using numpy.typing anywhere else), so, even if the meaning of npt is context-dependent, I don't think it's actually ambiguous.
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.
Thanks @neutrinoceros
In testing.py , We are importing both numpy.typing and numpy.testing , otherwise everything looks good on using npt as alias.
Should I stick to npt alias for numpy.typing and revert type hints changes in 'testing.py' ?
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.
ah, you mean this section?
Lines 1880 to 1897 in 2973f4d
| def _deprecated_numpy_testing_reexport(func): | |
| import numpy.testing as npt | |
| npt_func = getattr(npt, func.__name__) | |
| @wraps(npt_func) | |
| def retf(*args, **kwargs): | |
| __tracebackhide__ = True # Hide traceback for pytest | |
| issue_deprecation_warning( | |
| f"yt.testing.{func.__name__} is a pure re-export of " | |
| f"numpy.testing.{func.__name__}, it will stop working in the future. " | |
| "Please import this function directly from numpy instead.", | |
| since="4.2", | |
| stacklevel=3, | |
| ) | |
| return npt_func(*args, **kwargs) | |
| return retf |
ya, that's an unfortunate re-definition of npt in that function... I'd say (1) keep the import numpy.typing as npt at the top of the file and revert your changes and (2) adjust that testing import for clarity:
import numpy.testing as nptesting
npt_func = getattr(nptesting, func.__name__)Looks like there are a couple other minor spots where we do import numpy.testing as npt, but not in any of the files you're touching, so I'll submit a separate PR to fix those locations so we reserve npt for numpy.typing
Thanks! and FYI I'm hoping to take a detailed look this afternoon or tomorrow.
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.
still need to update this import
| import numpy.typing as nptype | |
| import numpy.typing as npt |
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.
looks pretty good! most of my comments are just a request to change all your imports to import numpy.typing as npt -- I didn't go through all the usages, so you'll need to update everywhere you've used nptype.
Also, there are a couple spots where I want to double check the dtype specification -- I will do that though since it will require a bit more familiarity with yt than I'd expect from a new contributor. Thanks for the work so far!
|
Hi @chrishavlin , I have already made changes in new PR. Please take a look in the new PR |
ef380c1 to
02a690c
Compare
chrishavlin
left a comment
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.
great! just one change that you missed
Co-authored-by: Chris Havlin <[email protected]>
Co-authored-by: Chris Havlin <[email protected]>
chrishavlin
left a comment
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.
Looks good to me! I'll try to drum up a second review. Thanks for the work!
yt/frontends/ramses/hilbert.py
Outdated
| ijk: "np.ndarray[Any, np.dtype[np.int64]]", bit_length: int | ||
| ) -> "np.ndarray[Any, np.dtype[np.float64]]": | ||
| ijk: "npt.NDArray[np.int64]", bit_length: int | ||
| ) -> "npt.NDArray[np.int64]": |
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.
@cphyc mind double checking my fix here? the hilbert index arrays will always be int arrays from what I can see.
(also if you're up for reviewing the rest of the PR it'd be welcome!)
| ds, | ||
| region: YTRegion, | ||
| LE: Optional["np.ndarray[Any, np.dtype[np.float64]]"] = None, | ||
| LE: Optional["npt.NDArray[np.float64]"] = None, |
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.
oh! nothing to change here, but I just remembered: the Any in the original np.ndarray[Any, np.dtype[np.float64]] is a reference to the array shape. So the update here is equivalent as npt.NDArray[dtype] is a type alias to np.ndarray[tuple[Any,...], dtype]
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.
|
@maverickcodex18 just FYI I just pushed changes to your branch to fix the failing pre-commit checks (with your updates there were some lingering unused imports of Thanks for you work! Hopefully we'll get a second review soon (I'll send some messages around). |
Thankyou @chrishavlin |
Walkthrough: Updating NumPy Type Hints
I have updated the type hints in yt to use
numpy.typing.NDArray(aliased asnpt.NDArray) instead ofnp.ndarray. This change improves type checking precision and adheres to modern NumPy typing standards. The alias ofnumpy.typingis updated fromnpttonptestingintesting.py.Changes Overview
The following files were modified to use
import numpy.typing as nptypeandnptype.NDArray:yt Core and Utilities
yt/visualizationyt/visualization/plot_window.pyyt/visualization/fixed_resolution.pyyt/visualization/plot_modifications.pyyt/frontendsyt/frontends/rockstar/data_structures.pyyt/frontends/artio/data_structures.pyyt/frontends/ramses/io.pyyt/frontends/ramses/hilbert.pyyt/frontends/ramses/particle_handlers.pyyt/frontends/stream/misc.pyVerification
I verified the changes by:
import numpy.typing as nptis replaced byimport numpy.typing as nptype.npt.NDArrayare replaced bynptype.NDArray.numpy.testing as npt.Files Checked But Not Modified
I inspected the following files and found they were either already compliant or only used
np.ndarrayin docstrings:yt/visualization/fixed_resolution_filters.py(Already compliant)yt/visualization/_handlers.py(Already compliant)yt/geometry/geometry_handler.py(Already compliant)yt/frontends/amrvac/io.py(Docstrings only)yt/utilities/lib/cykdtree/plot.py(Docstrings only)Excluded File where
np.ndarrayis presentFixes: #5144