-
-
Notifications
You must be signed in to change notification settings - Fork 701
Fixed mypy sp check guidelines #4887
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: develop
Are you sure you want to change the base?
Changes from all commits
ad17bc6
ba437d6
77863b8
5413828
f7beff0
fba51e2
fef8148
f5f6d50
e1b1aec
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -2,7 +2,6 @@ | |
| # Binary operator classes | ||
| # | ||
| from __future__ import annotations | ||
| import numbers | ||
|
|
||
| import numpy as np | ||
| import numpy.typing as npt | ||
|
|
@@ -35,7 +34,7 @@ def _preprocess_binary( | |
| right = pybamm.Vector(right) | ||
|
|
||
| # Check both left and right are pybamm Symbols | ||
| if not (isinstance(left, pybamm.Symbol) and isinstance(right, pybamm.Symbol)): | ||
| if not (isinstance(left, pybamm.Symbol) and isinstance(right, pybamm.Symbol)): # type: ignore[redundant-expr] | ||
| raise NotImplementedError( | ||
| f"BinaryOperator not implemented for symbols of type {type(left)} and {type(right)}" | ||
| ) | ||
|
|
@@ -114,6 +113,9 @@ def __str__(self): | |
| 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: | ||
| return self.__class__(self.name, left, right) # pragma: no cover | ||
|
|
||
| def create_copy( | ||
| self, | ||
| new_children: list[pybamm.Symbol] | None = None, | ||
|
|
@@ -128,7 +130,7 @@ def create_copy( | |
| children = self._children_for_copying(new_children) | ||
|
|
||
| if not perform_simplifications: | ||
| out = self.__class__(children[0], children[1]) | ||
| out = self._new_instance(children[0], children[1]) | ||
| else: | ||
| # creates a new instance using the overloaded binary operator to perform | ||
| # additional simplifications, rather than just calling the constructor | ||
|
|
@@ -225,6 +227,9 @@ def __init__( | |
| """See :meth:`pybamm.BinaryOperator.__init__()`.""" | ||
| super().__init__("**", left, right) | ||
|
|
||
| def _new_instance(self, left: pybamm.Symbol, right: pybamm.Symbol) -> pybamm.Symbol: | ||
| return Power(left, right) | ||
|
|
||
|
Comment on lines
+230
to
+232
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Why do we need new
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I've added new_instance because earlier when we were using __ 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 child classes of BinaryOperator which don't need to pass 3 arguments, so i thought it was better to make a new_instance method which can be overirded in child classes so should I update the PR description or think of some other approach?
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. And for broadcasts.py self.broadcast_domain was not defined in parent class they were params of child classes and _unary_new_copy function was getting called from child class instance always so I just overrided this function in child classes, apart from these changes all other changes are mostly type changes |
||
| def _diff(self, variable: pybamm.Symbol): | ||
| """See :meth:`pybamm.Symbol._diff()`.""" | ||
| # apply chain rule and power rule | ||
|
|
@@ -274,6 +279,9 @@ def __init__( | |
| """See :meth:`pybamm.BinaryOperator.__init__()`.""" | ||
| super().__init__("+", left, right) | ||
|
|
||
| def _new_instance(self, left: pybamm.Symbol, right: pybamm.Symbol) -> pybamm.Symbol: | ||
| return Addition(left, right) | ||
|
|
||
| def _diff(self, variable: pybamm.Symbol): | ||
| """See :meth:`pybamm.Symbol._diff()`.""" | ||
| return self.left.diff(variable) + self.right.diff(variable) | ||
|
|
@@ -301,6 +309,9 @@ def __init__( | |
|
|
||
| super().__init__("-", left, right) | ||
|
|
||
| def _new_instance(self, left: pybamm.Symbol, right: pybamm.Symbol) -> pybamm.Symbol: | ||
| return Subtraction(left, right) | ||
|
|
||
| def _diff(self, variable: pybamm.Symbol): | ||
| """See :meth:`pybamm.Symbol._diff()`.""" | ||
| return self.left.diff(variable) - self.right.diff(variable) | ||
|
|
@@ -330,6 +341,9 @@ def __init__( | |
|
|
||
| super().__init__("*", left, right) | ||
|
|
||
| def _new_instance(self, left: pybamm.Symbol, right: pybamm.Symbol) -> pybamm.Symbol: | ||
| return Multiplication(left, right) | ||
|
|
||
| def _diff(self, variable: pybamm.Symbol): | ||
| """See :meth:`pybamm.Symbol._diff()`.""" | ||
| # apply product rule | ||
|
|
@@ -370,6 +384,9 @@ def __init__( | |
| """See :meth:`pybamm.BinaryOperator.__init__()`.""" | ||
| super().__init__("@", left, right) | ||
|
|
||
| def _new_instance(self, left: pybamm.Symbol, right: pybamm.Symbol) -> pybamm.Symbol: | ||
| return MatrixMultiplication(left, right) # pragma: no cover | ||
|
|
||
| def diff(self, variable): | ||
| """See :meth:`pybamm.Symbol.diff()`.""" | ||
| # We shouldn't need this | ||
|
|
@@ -419,6 +436,9 @@ def __init__( | |
| """See :meth:`pybamm.BinaryOperator.__init__()`.""" | ||
| super().__init__("/", left, right) | ||
|
|
||
| def _new_instance(self, left: pybamm.Symbol, right: pybamm.Symbol) -> pybamm.Symbol: | ||
| return Division(left, right) | ||
|
|
||
| def _diff(self, variable: pybamm.Symbol): | ||
| """See :meth:`pybamm.Symbol._diff()`.""" | ||
| # apply quotient rule | ||
|
|
@@ -467,6 +487,9 @@ def __init__( | |
| """See :meth:`pybamm.BinaryOperator.__init__()`.""" | ||
| super().__init__("inner product", left, right) | ||
|
|
||
| def _new_instance(self, left: pybamm.Symbol, right: pybamm.Symbol) -> pybamm.Symbol: | ||
| return Inner(left, right) # pragma: no cover | ||
|
|
||
| def _diff(self, variable: pybamm.Symbol): | ||
| """See :meth:`pybamm.Symbol._diff()`.""" | ||
| # apply product rule | ||
|
|
@@ -544,6 +567,9 @@ def __init__( | |
| """See :meth:`pybamm.BinaryOperator.__init__()`.""" | ||
| super().__init__("==", left, right) | ||
|
|
||
| def _new_instance(self, left: pybamm.Symbol, right: pybamm.Symbol) -> pybamm.Symbol: | ||
| return Equality(left, right) | ||
|
|
||
| def diff(self, variable): | ||
| """See :meth:`pybamm.Symbol.diff()`.""" | ||
| # Equality should always be multiplied by something else so hopefully don't | ||
|
|
@@ -601,6 +627,10 @@ def __init__( | |
| ): | ||
| """See :meth:`pybamm.BinaryOperator.__init__()`.""" | ||
| super().__init__(name, left, right) | ||
| self.name = name | ||
|
|
||
| def _new_instance(self, left: pybamm.Symbol, right: pybamm.Symbol) -> pybamm.Symbol: | ||
| return _Heaviside(self.name, left, right) # pragma: no cover | ||
|
|
||
| def diff(self, variable): | ||
| """See :meth:`pybamm.Symbol.diff()`.""" | ||
|
|
@@ -679,6 +709,9 @@ def __init__( | |
| ): | ||
| super().__init__("%", left, right) | ||
|
|
||
| def _new_instance(self, left: pybamm.Symbol, right: pybamm.Symbol) -> pybamm.Symbol: | ||
| return Modulo(left, right) | ||
|
|
||
| def _diff(self, variable: pybamm.Symbol): | ||
| """See :meth:`pybamm.Symbol._diff()`.""" | ||
| # apply chain rule and power rule | ||
|
|
@@ -721,6 +754,9 @@ def __init__( | |
| ): | ||
| super().__init__("minimum", left, right) | ||
|
|
||
| def _new_instance(self, left: pybamm.Symbol, right: pybamm.Symbol) -> pybamm.Symbol: | ||
| return Minimum(left, right) | ||
|
|
||
| def __str__(self): | ||
| """See :meth:`pybamm.Symbol.__str__()`.""" | ||
| return f"minimum({self.left!s}, {self.right!s})" | ||
|
|
@@ -765,6 +801,9 @@ def __init__( | |
| ): | ||
| super().__init__("maximum", left, right) | ||
|
|
||
| def _new_instance(self, left: pybamm.Symbol, right: pybamm.Symbol) -> pybamm.Symbol: | ||
| return Maximum(left, right) | ||
|
|
||
| def __str__(self): | ||
| """See :meth:`pybamm.Symbol.__str__()`.""" | ||
| return f"maximum({self.left!s}, {self.right!s})" | ||
|
|
@@ -1539,7 +1578,7 @@ def source( | |
| corresponding to a source term in the bulk. | ||
| """ | ||
| # Broadcast if left is number | ||
| if isinstance(left, numbers.Number): | ||
| if isinstance(left, (int, float)): | ||
| left = pybamm.PrimaryBroadcast(left, "current collector") | ||
|
|
||
| # force type cast for mypy | ||
|
|
||
Uh oh!
There was an error while loading. Please reload this page.
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.
Given how big this PR is, it would actually be better to split it into multiple PRs, each one adding a new config option in pyproject.toml.
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.
ok so should I go ahead then and make a PR on one of the config first? or edit this one accordingly?
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.
You can keep this PR for one config, and add other configs in subsequent PRs.
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.
ok I'll create seperate PRs for different configs and then keep this one for the end, I think that would be faster for me
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 noticed many of the
warn_unreachableerror depends on errors fromenable_error_codeconfig, they're related to each other and their are total of 77 errors out of which 57 are fromenable_error_codeso I think creating a seperate PR would still be almost as big as this one, so should I still proceed with 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.
Yes, I think anything that reduces the diff and keeps this PR scoped to a specific change (or a few of them) would be great. Thanks for investigating!
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've created the PR with just
enable_error_codeconfig: #4891, sorry for the delay lab tests going on