Added mypy enable_error_code sp check guideline
#4891
Conversation
There was a problem hiding this comment.
Thanks, @Rishab87! Could you please merge develop and fix the tests?
…enable-error-code-1
|
@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 ReportAll modified and coverable lines are covered by tests ✅
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. 🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Thanks, @Rishab87! The tests pass now. Could you please comment why each of the change was carried out?
| @@ -255,7 +255,7 @@ def test_copy_with_computed_variables(self): | |||
|
|
|||
| assert ( | |||
| sol1._variables[k] == sol2._variables[k] for k in sol1._variables.keys() | |||
| ) | |||
| ) is not None | |||
There was a problem hiding this comment.
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()
)There was a problem hiding this comment.
aahh sorry I missed that, changed it now
| @@ -34,12 +34,12 @@ def process_2D(name, data): | |||
| D_s_n_data = process_2D("Negative particle diffusivity [m2.s-1]", df) | |||
|
|
|||
|
|
|||
| def D_s_n(sto, T): | |||
| def D_s_n_func(sto, T): | |||
There was a problem hiding this comment.
because their was a variable in this file with name D_s_n so mypy gave error function and variable both have same name
| @@ -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: | |||
There was a problem hiding this comment.
Could you please add some information on why this was added?
There was a problem hiding this comment.
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
|
@Saransh-cpp Thanks for the review, I've replied to the comments and changed the test |
Description
Added mypy
enable_error_codesp check guidelineRelated 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:
nox -s pre-commitnox -s testsnox -s doctests