-
-
Notifications
You must be signed in to change notification settings - Fork 713
Observe 0D custom variables after solving #5308
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
base: develop
Are you sure you want to change the base?
Conversation
Co-authored-by: Valentin Sulzer <[email protected]>
* Fix typo in concentration description in notebook * Add CHANGELOG.md entry for typo fix * Remove unneccesary changelog entry Co-authored-by: Agriya Khetarpal <[email protected]> --------- Co-authored-by: Agriya Khetarpal <[email protected]>
main -> develop
* fix `InputParameter` serialisation * Update CHANGELOG.md
…-fix Don't be too strict with func_args longer than symbol.children
Co-authored-by: pre-commit-ci[bot] <66853113+pre-commit-ci[bot]@users.noreply.github.com>
* feat: add`silence_sundial_warnings` solver option * refactor: `silence_sundials_warnings` -> `silence_sundials_errors`
Co-authored-by: pre-commit-ci[bot] <66853113+pre-commit-ci[bot]@users.noreply.github.com>
* raise `SolverError` at failure to init sundials * Update simulation.py * Update idaklu_solver.py * reuse `pybammsolvers` error messages * Update test_idaklu_solver.py * bump `pybammsolvers` * Update CHANGELOG.md * Update CHANGELOG.md Update CHANGELOG.md
| @@ -0,0 +1,248 @@ | |||
| import pybamm | |||
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.
SymbolReplacer and its test are copied from https://github.com/pybamm-team/pybamm-eis/blob/c88e134b28c50070dbf7b904c020c16b4b73e1d9/pybammeis/utils.py
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## develop #5308 +/- ##
===========================================
- Coverage 98.77% 98.69% -0.09%
===========================================
Files 321 322 +1
Lines 27746 27947 +201
===========================================
+ Hits 27406 27582 +176
- Misses 340 365 +25 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
valentinsulzer
left a comment
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 don't like the idea of adding the parameter values as an argument to the model, could we find a different way of doing this, e.g.
- pass parameter values to solution
- create a "parameterized model" type which is model + parameter values (as created by the simulation) and pass that to the solution in the all_models argument
2 is my preferred solution but it is a bigger change
| "parameter_values": ( | ||
| Serialise._serialise_parameter_values( | ||
| getattr(model, "_parameter_values", None) | ||
| ) | ||
| if getattr(model, "_parameter_values", None) is not None | ||
| else None | ||
| ), |
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 don't think adding parameter values as an attribute to the model is the right pattern, I could see this causing some bugs. It would be better to have a "parameterized model" object which is model + parameter values.
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.
Is this just here so we can access parameter values when processing a variable in observe? In that case could the parameter values be an optional argument of Solution instead (and throw an error in observe if not provided)?
| return d | ||
|
|
||
| @staticmethod | ||
| def _serialise_parameter_values( |
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.
why not use ParameterValues.to_json() ?
@valentinsulzer the reason I attached |
Description
Adds preliminary ability to lazily evaluate custom pybamm expressions after solving the simulation. E.g.,
This currently only supports 0D variables. It's not straightforward to support variables with meshes because only their discretized version is available at solution time. To support lazy observation of general variables (including the non-state variables), we would need to ensure that the original, non-discretized
model.variablesis not replaced with a discretized version.Type of change
Please add a line in the relevant section of CHANGELOG.md to document the change (include PR #)
Important checks:
Please confirm the following before marking the PR as ready for review:
nox -s pre-commitnox -s testsnox -s doctests