-
-
Notifications
You must be signed in to change notification settings - Fork 590
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
3937 events with idaklu output variables - solver edit #4300
3937 events with idaklu output variables - solver edit #4300
Conversation
… state vector slice
…pecified. Otherwise empty array.
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. |
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.
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 @pipliggins, looks good
@all-contributors please add @pipliggins for code and test |
I've put up a pull request to add @pipliggins! 🎉 |
) * 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>
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_event
attribute 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: