Added mypy ```enable_error_code``` sp check guideline by Rishab87 · Pull Request #4891 · pybamm-team/PyBaMM

Rishab87 · 2025-03-04T13:08:27Z

Description

Added mypy enable_error_code sp check guideline

Related to #3489, #4887

Type of change

Please add a line in the relevant section of CHANGELOG.md to document the change (include PR #)

Important checks:

Please confirm the following before marking the PR as ready for review:

No style issues: nox -s pre-commit
All tests pass: nox -s tests
The documentation builds: nox -s doctests
Code is commented for hard-to-understand areas
Tests added that prove fix is effective or that feature works

Saransh-cpp

Thanks, @Rishab87! Could you please merge develop and fix the tests?

…enable-error-code-1

Rishab87 · 2025-03-10T16:16:05Z

@Saransh-cpp I've merged the develop branch and all the tests pass locally, its weird that I haven't changed anything related to serialization still it fails, can you try re-running it? From what I'm able to interpret is failure is most likely due to a timing issue with how the filename is generated

codecov · 2025-03-10T19:43:10Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 98.70%. Comparing base (8f91615) to head (81f74bf).
Report is 4 commits behind head on develop.

Additional details and impacted files

@@             Coverage Diff             @@
##           develop    #4891      +/-   ##
===========================================
- Coverage    98.71%   98.70%   -0.01%     
===========================================
  Files          304      304              
  Lines        23509    23540      +31     
===========================================
+ Hits         23207    23236      +29     
- Misses         302      304       +2

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

❄ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

Saransh-cpp

Thanks, @Rishab87! The tests pass now. Could you please comment why each of the change was carried out?

Saransh-cpp · 2025-03-11T14:57:07Z

tests/unit/test_solvers/test_solution.py

        assert (
            sol1._variables[k] == sol2._variables[k] for k in sol1._variables.keys()
-        )
+        ) is not None


Nice! mypy caught a bad test. Your implementation changes the logic of the test and makes it not do anything useful. It should instead be changed into:

assert all( sol1._variables[k] == sol2._variables[k] for k in sol1._variables.keys() )

aahh sorry I missed that, changed it now

Saransh-cpp · 2025-03-11T14:58:26Z

examples/scripts/minimal_example_of_lookup_tables.py



-def D_s_n(sto, T):
+def D_s_n_func(sto, T):


Why was this changed?

because their was a variable in this file with name D_s_n so mypy gave error function and variable both have same name

I think the motive behind redefining the variable here was to show how one can pass both a constant value and a function as a parameter value. I would say we should revert this and not run mypy on examples at all (see my comment below).

Saransh-cpp · 2025-03-11T14:59:16Z

src/pybamm/expression_tree/binary_operators.py

            right_str = f"{self.right!s}"
        return f"{left_str} {self.name} {right_str}"

+    def _new_instance(self, left: pybamm.Symbol, right: pybamm.Symbol) -> pybamm.Symbol:


Could you please add some information on why this was added?

I've replaced self.__ class __ with new_instance because earlier when we were using self.__ class __ it showed a third arg was not getting passed:

error: Missing positional argument "right_child" in call to "BinaryOperator" [call-arg]

but this function was always getting called from instance of its child classes which don't need to pass 3 arguments, so i thought it was better to make a new_instance method which can be overrided in child classes

I've already added this in the PR description of previous sp-check-guidelines PR, should I add it here too? Or follow some different approach

Rishab87 · 2025-03-11T16:01:38Z

@Saransh-cpp Thanks for the review, I've replied to the comments and changed the test

Saransh-cpp

Thanks, @Rishab87! I think I wasn't clear before. I wanted you to comment on each of your change that edits the code logic with an explanation of why it is required. See my comments below -

examples/scripts/heat_equation.py

pyproject.toml

Saransh-cpp · 2025-03-19T10:01:54Z

examples/scripts/minimal_example_of_lookup_tables.py



-def D_s_n(sto, T):
+def D_s_n_func(sto, T):


I think the motive behind redefining the variable here was to show how one can pass both a constant value and a function as a parameter value. I would say we should revert this and not run mypy on examples at all (see my comment below).

Saransh-cpp · 2025-03-19T10:03:06Z

tests/unit/test_expression_tree/test_operations/test_copy.py

                a * b,
                a / b,
                a**b,
+                b % a,


Why was this added?

to ensure coverage, as told in my earlier comment too above:

I've replaced self.__ class __ with new_instance because earlier when we were using self.__ class __ it showed a third arg was not getting passed:
error: Missing positional argument "right_child" in call to "BinaryOperator" [call-arg]
but this function was always getting called from instance of its child classes which don't need to pass 3 arguments, so i thought it was better to make a new_instance method which can be overided in child classes

due to this many new_instance functions have been added for modulo and other operations, we can use pragma no cover too instead of adding this in test

Saransh-cpp · 2025-03-19T10:04:41Z

src/pybamm/util.py

+        if not isinstance(keys, (str, list)) or not all(  # type: ignore[redundant-expr]
            isinstance(k, str) for k in keys


Can we simplify this expression instead of ignoring the warning?

The problem here is that keys type is str | list[str], so we if we remove this ignore, it will throw an error reduntant expression as according to the types, there is no point of checking whether keys list[str] but if we remove this if statement, few test cases fail as in those test cases we're intentionally passing other types of values, so I thought it would be best to ignore it.

Saransh-cpp · 2025-03-19T10:18:59Z

src/pybamm/models/base_model.py

        self.use_jacobian = True
        self.convert_to_format = "casadi"

+        self.calculate_sensitivities = []


Where is this variable coming from?

In base solver got this error:

src/pybamm/solvers/base_solver.py:1124: error: "BaseModel" has no attribute "calculate_sensitivities" [attr-defined]

that's why I added this

Saransh-cpp · 2025-03-19T10:19:42Z

src/pybamm/solvers/base_solver.py


        ninputs = len(model.calculate_sensitivities)
-        initial_conditions = tuple([] for _ in range(ninputs))
+        initial_conditions: tuple = tuple([] for _ in range(ninputs))


Why do we need the explicit tuple?

Because mypy was unable to infer it:

src/pybamm/solvers/base_solver.py:1125: error: Need type annotation for "initial_conditions" [var-annotated]

I think mypy has difficulty inferring that the tuple contains lists of a particular type

src/pybamm/solvers/idaklu_jax.py

src/pybamm/solvers/processed_variable_time_integral.py

Rishab87 · 2025-03-31T16:22:02Z

@Saransh-cpp , thanks for the review and sorry for the late reply, I was busy with my mid sem exams along with my proposal, I will go through the comments and fix these issues.

Co-authored-by: Saransh Chopra <saransh0701@gmail.com>

…ble-error-code-1

into enable-error-code-1

…enable-error-code-1

Rishab87 · 2025-04-09T17:59:50Z

@Saransh-cpp , I've made the changes and replied to the comments, can you have a look?

added enable_error_code config in mypy

de39912

Rishab87 requested a review from a team as a code owner March 4, 2025 13:08

Rishab87 mentioned this pull request Mar 4, 2025

Fixed mypy sp check guidelines #4887

Open

5 tasks

Saransh-cpp requested changes Mar 10, 2025

View reviewed changes

Merge branch 'develop' of https://github.com/pybamm-team/PyBaMM into …

2d7a327

…enable-error-code-1

Saransh-cpp self-requested a review March 11, 2025 14:31

Saransh-cpp requested changes Mar 11, 2025

View reviewed changes

test-fix

81f74bf

Rishab87 requested a review from Saransh-cpp March 11, 2025 16:01

Saransh-cpp requested changes Mar 19, 2025

View reviewed changes

Rishab87 and others added 7 commits April 9, 2025 21:13

Update src/pybamm/solvers/summary_variable.py

59ac4f3

Co-authored-by: Saransh Chopra <saransh0701@gmail.com>

Update src/pybamm/solvers/idaklu_jax.py

a3895c7

Co-authored-by: Saransh Chopra <saransh0701@gmail.com>

Merge branch 'main' of https://github.com/pybamm-team/PyBaMM into ena…

1ae758b

…ble-error-code-1

Merge branch 'enable-error-code-1' of https://github.com/Rishab87/PyBaMM

b04f766

into enable-error-code-1

reverted examples

dfdd7ff

made changes according to comments

160f279

Merge branch 'develop' of https://github.com/pybamm-team/PyBaMM into …

0a42b57

…enable-error-code-1

Rishab87 requested a review from Saransh-cpp April 9, 2025 17:59

		if not isinstance(keys, (str, list)) or not all( # type: ignore[redundant-expr]
		isinstance(k, str) for k in keys

Uh oh!

Conversation

Rishab87 commented Mar 4, 2025

Description

Type of change

Important checks:

Uh oh!

Saransh-cpp left a comment

Choose a reason for hiding this comment

Uh oh!

Rishab87 commented Mar 10, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

codecov bot commented Mar 10, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

Saransh-cpp 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!

Rishab87 Mar 11, 2025 • 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!

Rishab87 commented Mar 11, 2025

Uh oh!

Saransh-cpp left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

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!

Rishab87 Apr 9, 2025 • 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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Rishab87 commented Mar 31, 2025

Uh oh!

Rishab87 commented Apr 9, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

Rishab87 commented Mar 10, 2025 •

edited

Loading

codecov bot commented Mar 10, 2025 •

edited

Loading

Rishab87 Mar 11, 2025 •

edited

Loading

Rishab87 Apr 9, 2025 •

edited

Loading