-
-
Notifications
You must be signed in to change notification settings - Fork 554
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
Fixes for sympy update #4253
Fixes for sympy update #4253
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## develop #4253 +/- ##
===========================================
- Coverage 99.55% 99.54% -0.02%
===========================================
Files 288 288
Lines 21886 21886
===========================================
- Hits 21789 21786 -3
- Misses 97 100 +3 ☔ View full report in Codecov by Sentry. |
Looks good, happy to approve once coverage passes. |
@brosaplanella I am pretty sure that failure is a false positive. It is the same failure in #4252, which does not change any code |
Description
SymPy updated and seems to no longer recognize PyBaMM symbols as strings.
It gets to this block (N.B. warning text was edited for readability):
Then fails due to an exception. Even if it did convert to a string, it would have caused a deprecation warning.
Failing tests looked like this:
There seemed to be two options:
pybamm.Symbol._sympy_()
method. This would be basically identical topybamm.Symbol.to_equation()
, and would not be a good test.pybamm.Symbol.to_equation()
by confirming thatpybamm.Symbol.print_name()
returns the original name, but could be a bit redundant.I went with option (2) since it keeps the tests working, but it could be good to determine if
pybamm.Symbol.print_name()
andpybamm.Symbol.to_equation()
could be just replaced by apybamm.Symbol._sympy_()
method.Type of change
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: