-
-
Notifications
You must be signed in to change notification settings - Fork 553
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
IDA adaptive time stepping #4351
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## develop #4351 +/- ##
===========================================
- Coverage 99.50% 99.46% -0.05%
===========================================
Files 289 289
Lines 22146 22200 +54
===========================================
+ Hits 22037 22081 +44
- Misses 109 119 +10 ☔ View full report in Codecov by Sentry. 🚨 Try these New Features:
|
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.
Looks good to me other than some very minor comments. I have a feeling there are several bits of code we'll be able to refactor once this is in as the default everywhere but that can be a job for later. As usual will leave the C++ code to others to review
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.
Really great work @MarcBerliner, this looks excellent and some really great speed-ups! :) I've suggested a few changes below that need to be fixed but mostly minor. One general question I had: many of the idaklu tests use a dense t_eval and an identical t_interp, which is now the "slow" way of running the solver. When not specificly testing the interpolation code, I would use the most efficient / standard way of running solver.solve
, which I believe would be t_eval = [start, stop]
and t_interp = None
or t_interp = np.linspace(start, stop, N)
?
@martinjrobins good point. The most efficient way to run it is using the Once this PR merges, we can fully migrate all the tests to using |
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 @MarcBerliner, just a few more minor changes below
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.
Looks great, thanks @MarcBerliner
Description
This PR makes several changes to adaptive time-stepping in IDA. Previously, the
t_eval
decided both the points at which we store the solution and the points to explicitly stop the IDA solver. Now, the time-stepping has been broken down intot_eval
(which retains the same behavior) andt_interp
, which uses IDA's internal Hermite interpolator to efficiently store the solution. Additionally, we default to saving the adaptive time steps, which gives us full-order solution accuracy without negatively impacting the solve speed with periods or denset_eval
vectors.This PR cleans up the main
solve
loop of the IDA solver and clearly delineates saving the full solutiony
vs. specific output variables.Example: adaptive time outputs (new) vs. fixed outputs
The above picture shows the difference in resolution between the new adaptive time-stepping scheme and the previous approach without affecting the performance. The markers show every data point available in
sol.t
Period/drive cycles improvements
The above figure shows the
solve
time for a full C/10 discharge. The new IDA solver is strictly faster than the old IDA and the Casadi solver. For the SPM, the IDA vs. Casadi timings are closer because IDA is a DAE solver while Casadi is using an ODE solver. Adding CVODE to pybamm could be future work to further improve our ODE speed.For small periods, the new IDA solver is about an order of magnitude faster than the old IDA. For large periods, it's about half an order of magnitude faster than Casadi.
In a parameter estimation context, you can also post-interpolate the adaptive time-stepping solution without specifying a
t_interp
orperiod
for just the variable of interest (such as voltage) which will dramatically improve the performance.Adaptive time-stepping (no period)
When
t_eval
or aperiod
is not set for an experiment, the new IDA is strictly faster than the previous IDA. Additionally, adaptive time-stepping has much better performance for slower simulations that previously used a denset_eval
vector by default.Fixes #4312
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
(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: