changed np.nparray to np.typing.NDArray #4526

sanjayh-2022 · 2024-10-18T14:46:00Z

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.

New feature (non-breaking change which adds functionality)
Optimization (back-end change that speeds up the code)
Bug fix (non-breaking change which fixes an issue)

Key checklist:

No style issues: $ 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)
All tests pass: $ python run-tests.py --all (or $ nox -s tests)
The documentation builds: $ 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:

Code is commented, particularly in hard-to-understand areas
Tests added that prove fix is effective or that feature works

agriyakhetarpal

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?

kratman · 2024-10-18T15:02:14Z

src/pybamm/solvers/idaklu_jax.py

        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).


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.

sanjayh-2022 · 2024-10-19T07:31:47Z

I have made the required changes. please review my changes and let me know if any changes are required

arjxn-py

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

agriyakhetarpal · 2024-10-21T13:44:14Z

You can fix the pre-commit CI and the style failures by installing pre-commit locally, please read through our Contributing guide: https://docs.pybamm.org/en/latest/source/user_guide/contributing.html#installing-and-using-pre-commit. You may also use nox -s pre-commit to do the same thing.

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.

sanjayh-2022 · 2024-10-28T05:39:45Z

i have added import numpy.typing as npt after import numpy as np. Please review the changes

Saransh-cpp

Thanks, @sanjayh-2022! Please see my comments below.

Saransh-cpp · 2024-10-28T17:16:23Z

src/pybamm/experiment/step/base_step.py

@@ -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)


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.

Saransh-cpp · 2024-10-28T17:16:29Z

src/pybamm/discretisations/discretisation.py

@@ -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):


Saransh-cpp · 2024-10-28T17:16:42Z

src/pybamm/experiment/step/base_step.py

@@ -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):


Saransh-cpp · 2024-10-28T17:16:50Z

src/pybamm/expression_tree/array.py

@@ -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):


Saransh-cpp · 2024-10-28T17:17:07Z

src/pybamm/expression_tree/binary_operators.py

@@ -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):


For all isinstance calls basically

Saransh-cpp · 2024-10-28T17:20:00Z

src/pybamm/expression_tree/array.py

+import numpy as np
+import numpy.typing as npt
+


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).

changed np.nparray to np.typing.NDArray

9a2fd3c

sanjayh-2022 requested review from brosaplanella, Saransh-cpp, agriyakhetarpal, arjxn-py, a team, martinjrobins, rtimms, valentinsulzer, TomTranter, MarcBerliner, DrSOKane and kratman as code owners October 18, 2024 14:46

style: pre-commit fixes

60ef135

agriyakhetarpal requested changes Oct 18, 2024

View reviewed changes

kratman reviewed Oct 18, 2024

View reviewed changes

sanjayh-2022 and others added 3 commits October 19, 2024 12:55

imported numpy.typing as npt and make the chnges accordingly

b6c7170

style: pre-commit fixes

ee16030

Merge branch 'pybamm-team:develop' into develop

1126ce4

sanjayh-2022 closed this Oct 19, 2024

sanjayh-2022 reopened this Oct 19, 2024

arjxn-py requested changes Oct 20, 2024

View reviewed changes

sanjayh-2022 and others added 2 commits October 28, 2024 10:26

added import numpy.typing as npt after import numpy as np

fc4041b

Merge branch 'pybamm-team:develop' into develop

f80584f

Saransh-cpp requested changes Oct 28, 2024

View reviewed changes

Merge branch 'develop' into develop

7258e1f

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

changed np.nparray to np.typing.NDArray #4526

changed np.nparray to np.typing.NDArray #4526

sanjayh-2022 commented Oct 18, 2024 •

edited

Loading

agriyakhetarpal left a comment

kratman Oct 18, 2024

sanjayh-2022 commented Oct 19, 2024

arjxn-py left a comment

agriyakhetarpal commented Oct 21, 2024

sanjayh-2022 commented Oct 28, 2024

Saransh-cpp left a comment

Saransh-cpp Oct 28, 2024

Saransh-cpp Oct 28, 2024

Saransh-cpp Oct 28, 2024

Saransh-cpp Oct 28, 2024

Saransh-cpp Oct 28, 2024

Saransh-cpp Oct 28, 2024

changed np.nparray to np.typing.NDArray #4526

Are you sure you want to change the base?

changed np.nparray to np.typing.NDArray #4526

Conversation

sanjayh-2022 commented Oct 18, 2024 • edited Loading

Description

Type of change

Key checklist:

Further checks:

agriyakhetarpal left a comment

Choose a reason for hiding this comment

kratman Oct 18, 2024

Choose a reason for hiding this comment

sanjayh-2022 commented Oct 19, 2024

arjxn-py left a comment

Choose a reason for hiding this comment

agriyakhetarpal commented Oct 21, 2024

sanjayh-2022 commented Oct 28, 2024

Saransh-cpp left a comment

Choose a reason for hiding this comment

Saransh-cpp Oct 28, 2024

Choose a reason for hiding this comment

Saransh-cpp Oct 28, 2024

Choose a reason for hiding this comment

Saransh-cpp Oct 28, 2024

Choose a reason for hiding this comment

Saransh-cpp Oct 28, 2024

Choose a reason for hiding this comment

Saransh-cpp Oct 28, 2024

Choose a reason for hiding this comment

Saransh-cpp Oct 28, 2024

Choose a reason for hiding this comment

sanjayh-2022 commented Oct 18, 2024 •

edited

Loading