-
Notifications
You must be signed in to change notification settings - Fork 30
New issue
Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? # to your account
Adds "fast with events"
as default echem solver
#301
Adds "fast with events"
as default echem solver
#301
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## develop #301 +/- ##
========================================
Coverage 95.79% 95.80%
========================================
Files 38 38
Lines 2045 2048 +3
========================================
+ Hits 1959 1962 +3
Misses 86 86 ☔ View full report in Codecov by Sentry. |
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.
Thanks, @BradyPlanden, would you also consider using numpy.testing.assert_allclose
instead – it's the more recommended option nowadays, maybe not here, but in another PR, or here if you feel like it? The tolerances to check for would have to be adjusted in some test cases. I think the changes look good, though, and I'll let others review it too.
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.
changes look good, buth how have you found the "fast with events" solver compare against the idaklu solver? If events are important (e.g. in an experiment) the idaklu solver can be quite a bit faster.
The IDAKLU solver was tried in #176 and #217 and it's partially working for Unix-like platforms (with a non-repaired wheel and when built from source, so that's not ideal). Windows support is probably still going to be broken for a while, because of the reasons described on the PyBaMM side where it doesn't work with |
fair enough! happy to stick with casadi then! |
Description
This PR updates the default solver for the echem models to include the
"fast with events"
mode. More information on this solver can be found:https://github.com/pybamm-team/PyBaMM/blob/develop/pybamm/solvers/casadi_solver.py
https://docs.pybamm.org/en/stable/source/api/solvers/casadi_solver.html#casadi-solver
This is currently listed as
experimental
within the PyBaMM documentation, so I'm open to input on whether this should be blocked for a bit to ensure robustness.Issue reference
Fixes #300
Review
Before you mark your PR as ready for review, please ensure that you've considered the following:
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)$ nox -s tests
$ nox -s doctest
You can run integration tests, unit tests, and doctests together at once, using
$ nox -s quick
.Further checks:
Thank you for contributing to our project! Your efforts help us to deliver great software.