-
-
Notifications
You must be signed in to change notification settings - Fork 551
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
Improve consistent initialization speed and robustness #4301
Improve consistent initialization speed and robustness #4301
Conversation
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 at a superficial level. Much overdue clean up and good to know why we've been getting all these IDACalcIC errors.
I'll let others (@martinjrobins / @jsbrittain ) who have done more on the IDA KLU solver review in more detail
Check out this pull request on See visual diffs & provide feedback on Jupyter Notebooks. Powered by ReviewNB |
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## develop #4301 +/- ##
===========================================
+ Coverage 99.45% 99.50% +0.04%
===========================================
Files 288 288
Lines 22091 22079 -12
===========================================
- Hits 21971 21970 -1
+ Misses 120 109 -11 ☔ 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 @MarcBerliner, this looks great, thank you for your effort on this. In the future, as per the CONTRIBUTING.md can you please make sure to first open an issue before you start on a piece of work so that it can be discussed with the other developers. It would be great also to get your input on #3910. Please feel free to add additional items that you think are important for the solvers going forward, or edit existing items. I would like this to be an evolving workplan so we can coordinate everyone's work on the solvers.
pybamm/solvers/base_solver.py
Outdated
and model.mass_matrix.shape[0] > model.len_rhs_and_alg | ||
): | ||
if model.mass_matrix_inv is not None: | ||
if has_mass_matrix and model.mass_matrix.shape[0] > model.len_rhs_and_alg: |
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.
can we just get rid of this if statement? on the face of it, it seems unnessessary
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.
I added a check that early returns if we don't have a mass matrix (necessary for the DummySolver
tests),
if not has_mass_matrix:
return
and removed all the following has_mass_matrix
checks.
pybamm/solvers/idaklu_solver.py
Outdated
# calculate the time derivatives of the differential equations | ||
# for semi-explicit DAEs | ||
calc_ydot0 = model.mass_matrix_inv is not None | ||
if model.len_rhs > 0 and calc_ydot0: |
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.
its a bit unclear in what situation the solver would calculate ydot0. Presumably we will always want to calculate ydot0 if there are any differential equations (i.e. model.len_rhs > 0)? so the model.mass_matrix_inv should be available in this case (and we should error if not).
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.
Good point - we will always have a mass matrix and its inverse if len_rhs
is nonzero. Removing calc_ydot0
attempt to fix the iree tols
Thanks for the quick feedback @martinjrobins. Going forward, I'll make sure to keep you all informed of my planned work. I'll add a few comments to the solver roadmap. |
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, thanks @MarcBerliner
Description
The
IDA
solver takes in initial guesses fory0
andydot0
. Currently, we explicitly calculatey0
using an algebraic solver before passing it toIDA
. However, we do not estimateydot0
- we just set it tonp.zeros_like(y0)
and rely onIDA
to determine it. This PR makes a few changes to consistent initialization:ydot0
using the mass matrix inverse. TheIDA
solver is set up only for implicit DAEs, so it cannot properly utilize the mass matrix in initializing the system. As a result, it wastes a lot of time re-computing the consistent initialization fory0
andydot0
inIDACalcIC
. We can improve this by isolating the differential section of our system of equations and explicitly computingydot0
for the rhs,where the subscript
IDACalcIC
, which occasionally fails.initial_conditions
andconsistent_initialization
. I found that there was not a clear distinction between these ideas in the solver code. In this update,initial_conditions
refers to the initial values and guesses provided by the solver or from a previous solution. Initial conditions likely will not satisfy the system of equations att0
, whileconsistent_initialization
refers to the solution that satisfies the system of equations att0
. In some cases, we computed theinitial_conditions
more than once for a single solve, which were removed.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: