-
-
Notifications
You must be signed in to change notification settings - Fork 553
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
changed np.nparray to np.typing.NDArray #4526
base: develop
Are you sure you want to change the base?
Conversation
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, @sanjayh-2022! Could you instead the NumPy typing submodule separately using the import numpy.typing as npt
statement in addition to import np
and use npt.NDArray
here instead, like @Saransh-cpp has previously suggested?
src/pybamm/solvers/idaklu_jax.py
Outdated
inputs: Union[dict, None] = None, | ||
output_variables: Union[list[str], None] = None, | ||
): | ||
"""Helper function to compute the gradient of a jaxified expression | ||
|
||
Returns a numeric (np.ndarray) object (not a JAX expression). | ||
Returns a numeric (np.typing.NDArray) object (not a JAX expression). |
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.
I think in comments we would probably want to keep it np.ndarray instead of the typing object. My understanding is that the np.typing module is just for type hints.
I have made the required changes. please review my changes and let me know if any changes are required |
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 @sanjayh-2022, I'd once again have to request changes as you've to also be careful about the order of imports in a file. It'd be nicer to import numpy.typing as npt
not on the top of the file but the line after import numpy as np
You can fix the pre-commit CI and the style failures by installing In particular, the following lines: pip install pre-commit
pre-commit install will run the style checks before any code is committed, and will hence prevent you from pushing code that fails these style checks. |
i have added import numpy.typing as npt after import numpy as np. Please review the changes |
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, @sanjayh-2022! Please see my comments below.
@@ -74,7 +76,7 @@ def __init__( | |||
self.input_duration = duration | |||
self.input_value = value | |||
# Check if drive cycle | |||
is_drive_cycle = isinstance(value, np.ndarray) | |||
is_drive_cycle = isinstance(value, npt.NDArray) |
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.
This should not be changed as we are checking if value
is an instance of np.ndarray
. We could have changed it if we were checking the type of the value
, but we don't want that.
@@ -1012,7 +1014,7 @@ def check_initial_conditions(self, model): | |||
# Individual | |||
for var, eqn in model.initial_conditions.items(): | |||
ic_eval = eqn.evaluate(t=0, inputs="shape test") | |||
if not isinstance(ic_eval, np.ndarray): | |||
if not isinstance(ic_eval, npt.NDArray): |
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.
Ditto
@@ -260,7 +262,7 @@ def default_duration(self, value): | |||
Default duration for the step is one day (24 hours) or the duration of the | |||
drive cycle | |||
""" | |||
if isinstance(value, np.ndarray): | |||
if isinstance(value, npt.NDArray): |
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.
Ditto
@@ -165,7 +167,7 @@ def to_json(self): | |||
Method to serialise an Array object into JSON. | |||
""" | |||
|
|||
if isinstance(self.entries, np.ndarray): | |||
if isinstance(self.entries, npt.NDArray): |
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.
Ditto
@@ -22,13 +24,13 @@ def _preprocess_binary( | |||
) -> tuple[pybamm.Symbol, pybamm.Symbol]: | |||
if isinstance(left, (float, int, np.number)): | |||
left = pybamm.Scalar(left) | |||
elif isinstance(left, np.ndarray): | |||
elif isinstance(left, npt.NDArray): |
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.
For all isinstance
calls basically
import numpy as np | ||
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.
numpy
imports should not be at the top of the file. The order should be -
- module-level comment/docstring
from __future__ import annotations
- All other imports
This will fix the failing style check (which you can also run locally using instructions provided above).
Description
Please include a summary of the change and which issue is fixed. Please also include relevant motivation and context. List any dependencies that are required for this change.
Fixes #4512
Type of change
Please add a line in the relevant section of CHANGELOG.md to document the change (include PR #) - note reverse order of PR #s. If necessary, also add to the list of breaking changes.
Key checklist:
$ pre-commit run
(or$ nox -s pre-commit
) (see CONTRIBUTING.md for how to set this up to run automatically when committing locally, in just two lines of code)$ python run-tests.py --all
(or$ nox -s tests
)$ python run-tests.py --doctest
(or$ nox -s doctests
)You can run integration tests, unit tests, and doctests together at once, using
$ python run-tests.py --quick
(or$ nox -s quick
).Further checks: