Add max_step arg in basesolver
#3106
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## develop #3106 +/- ##
========================================
Coverage 99.60% 99.60%
========================================
Files 259 259
Lines 21287 21309 +22
========================================
+ Hits 21203 21225 +22
Misses 84 84 ☔ View full report in Codecov by Sentry. |
|
@arjxn-py are you still working on this? |
|
Yes, i'll try to finish this up asap. |
|
It looks like Chocolatey community repositories are under maintenance, which explains the Windows job failures. |
arjxn-py
requested review from
kratman,
agriyakhetarpal,
Saransh-cpp,
brosaplanella,
martinjrobins,
rtimms,
valentinsulzer and
TomTranter
as code owners
|
@rtimms can you give me some input about setting |
|
Check out this pull request on See visual diffs & provide feedback on Jupyter Notebooks. Powered by ReviewNB |
|
Should also do changelog for this, right? |
Yup |
|
@rtimms I'd be happy to have your review & input on this. Thanks. |
There was a problem hiding this comment.
In all of the solver classes you need to pass max_step to the actual call to solve in the appropriate way inside the _integrate method. Right now passing max_step doesn't do anything. E.g. in ScipySolver you need to pass max_step as a keyword argument to solve_ivp
| @@ -24,14 +25,18 @@ class AlgebraicSolver(pybamm.BaseSolver): | |||
| specified in the form "lsq_methodname" | |||
| tol : float, optional | |||
| The tolerance for the solver (default is 1e-6). | |||
| max_step : float, optional | |||
There was a problem hiding this comment.
the algebraic solver doesn't step in time so it doesn't make sense for it to be an argument here
| @@ -4,6 +4,7 @@ | |||
| import casadi | |||
| import pybamm | |||
| import numpy as np | |||
| from .base_solver import validate_max_step | |||
|
|
|||
|
|
|||
| class CasadiAlgebraicSolver(pybamm.BaseSolver): | |||
| @@ -16,6 +16,13 @@ | |||
| from pybamm.expression_tree.binary_operators import _Heaviside | |||
|
|
|||
|
|
|||
| def validate_max_step(max_step): | |||
There was a problem hiding this comment.
we don't validate other arguments such as tolerances. my guess is that all the solvers will have their own check to make sure max step makes sense, so maybe we should remove this?
| max_step_decrease_count : float, optional | ||
| The maximum number of times step size can be decreased before an error is | ||
| raised. Default is 5. | ||
| dt_max : float, optional | ||
| dt_event : float, optional |
There was a problem hiding this comment.
I agree dt_event is a better name for this. Can you make it so that the solver also still accepts dt_max and raises a DeprecationWarning?
|
Closing since this is superseded by #4673. |
Description
Please include a summary of the change and which issue is fixed. Please also include relevant motivation and context. List any dependencies that are required for this change.
Fixes #2253
Type of change
Please add a line in the relevant section of CHANGELOG.md to document the change (include PR #) - note reverse order of PR #s. If necessary, also add to the list of breaking changes.
Key checklist:
$ pre-commit run(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$ python run-tests.py --doctestYou can run unit and doctests together at once, using
$ python run-tests.py --quick.Further checks: