-
-
Notifications
You must be signed in to change notification settings - Fork 517
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 #4150
3937 events with idaklu output variables #4150
Conversation
… for specified output_variables
…ables in place of state vector
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## develop #4150 +/- ##
===========================================
+ Coverage 99.55% 99.58% +0.02%
===========================================
Files 288 288
Lines 21790 21825 +35
===========================================
+ Hits 21693 21734 +41
+ Misses 97 91 -6 ☔ View full report in Codecov by Sentry. |
if v in event_children: | ||
required_vars.append(k) | ||
joined_vars = "\n".join(required_vars) | ||
raise ValueError( |
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.
Would it be better to raise a warning here rather than an Error, as this lets the user decide whether identification of the specific termination event is worth the expense of adding multiple additional variables (and their sensitivities) to the solution.
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.
Raising a warning and filling in the termination event with an empty string/"unknown" or similar would work for single simulations, but when used with an experiment the termination event is required to check when to move through experiment steps. This would require adding in different behaviour for using the solver with/without an experiment, perhaps auto-adding appropriate output variables when used with experiments. Currently sensitivities can't be used with experiments so there's minimal change to the computational expense.
An alternative implementation to consider in the future might be to change the IDAKLU solver output to include the state vector for the final timestep in the return value. This shouldn't impact the memory usage, and would allow calculation of any termination events using the pre-existing methods without the additional overhead of storing and calculating unwanted output variables/sensitivities during the solve.
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.
Sounds good!
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.
An alternative implementation to consider in the future might be to change the IDAKLU solver output to include the state vector for the final timestep in the return value. This shouldn't impact the memory usage, and would allow calculation of any termination events using the pre-existing methods without the additional overhead of storing and calculating unwanted output variables/sensitivities during the solve.
This is a great solution for all solvers since the final state vector is required to initialize the next step in an experiment.
inputs=solution.all_inputs[-1], | ||
) | ||
continue | ||
except ValueError as e: # pragma: no cover |
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.
Likewise, is this cause for a critical failure? I would consider replacing with a warning.
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.
See above
# Check that it's actually an event | ||
if final_event_values[termination_event] > 0.1: # pragma: no cover | ||
# Hard to test this | ||
raise pybamm.SolverError( |
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.
dito.
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.
This behaviour is copied straight over from the base function in base_solver.py
, assume it should stay the same
Description
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, not the entire state vector, so the events could not be evaluated outside the solver.
This PR adds a check to see if the events specified in a model can be calculated using the values of the output variables rather than the state vector. If they can, the events are calculated using these values after the solve. If not, an error is raised warning that events will not be able to be calculated with the current output variables, and provides a list of variables to choose from to add to the output_variables list, e.g.:
raises
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: