3937 events with idaklu output variables - solver edit#4300
Merged
martinjrobins merged 7 commits intoJul 30, 2024
Conversation
… state vector slice
…pecified. Otherwise empty array.
Contributor
Author
|
As mentioned in the previous PR (#4150 (comment)), specifying output variables in the IDAKLU solver doesn't currently work with experiments. This bug fix should enable that behaviour to be added, but I think it should be a separate issue/PR for a new feature, rather than added to this one. |
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## develop #4300 +/- ##
===========================================
- Coverage 99.45% 99.45% -0.01%
===========================================
Files 288 288
Lines 22092 22091 -1
===========================================
- Hits 21972 21971 -1
Misses 120 120 ☔ View full report in Codecov by Sentry. |
martinjrobins
approved these changes
Jul 30, 2024
martinjrobins
left a comment
Contributor
There was a problem hiding this comment.
thanks @pipliggins, looks good
Contributor
|
@all-contributors please add @pipliggins for code and test |
Contributor
|
I've put up a pull request to add @pipliggins! 🎉 |
js1tr3
pushed a commit
to js1tr3/PyBaMM
that referenced
this pull request
Aug 12, 2024
) * Add test that fails * Remove duplicate attribute assignment * IDAKLU solver returns additional y_term variable containing the final state vector slice * Add final state vector slice to python-idaklu, tests pass * Reduce memory load so y_term is only filled if output variables are specified. Otherwise empty array. * Edit changelog --------- Co-authored-by: Martin Robinson <martinjrobins@gmail.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description
This is an alternative to #4150 (old pull request will be closed)
Simulations using the IDAKLU solver with output variables specified which terminated with an event rather than the final time step would crash trying to find the termination event.
Termination events are found in the Solution class by using the state vector of the final time-step to evaluate each potential termination event and find the minimum (the event which caused the termination). When output variables are specified in the IDAKLU solver only those variables are output in
y_out, not the entire state vector, so the events could not be evaluated outside the solver.This PR edits the IDAKLU solver to add an additional return value containing the state vector for the final timestep only ('y_term') when output variables are specified. This value is used to fill the
Solution.y_eventattribute and allows termination events to be calculated as normal, while not impacting the performance advantages of selecting only a few output variables. When output variables are not specified, this returns an empty array as it copies part of the provided state vector.Fixes #3937
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: