-
-
Notifications
You must be signed in to change notification settings - Fork 572
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
Revert lru_cache to functional form; Add lru-dict for integrators #2823
Conversation
a2518cf
to
25936fc
Compare
This comment was marked as outdated.
This comment was marked as outdated.
55880c2
to
04170e3
Compare
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! Just a small comment on the CHANGELOG.
CHANGELOG.md
Outdated
@@ -8,13 +8,15 @@ | |||
- Added "Open-circuit voltage [V]", which is the open-circuit voltage as calculated from the bulk particle concentrations. The old variable "Measured open circuit voltage [V]", which referred to the open-circuit potential as calculated from the surface particle concentrations, has been renamed to "Surface open-circuit voltage [V]". ([#2740](https://github.com/pybamm-team/PyBaMM/pull/2740)) | |||
- Added an example for `plot_voltage_components`, explaining what the different voltage components are. ([#2740](https://github.com/pybamm-team/PyBaMM/pull/2740)) | |||
- Added method to calculate maximum theoretical energy. ([#2777](https://github.com/pybamm-team/PyBaMM/pull/2777)) and add to summary variables ([#2781](https://github.com/pybamm-team/PyBaMM/pull/2781)) | |||
- Added option to limit the number of integrators stored in CasadiSolver, which is particularly relevant when running simulations back-to-back [#2823](https://github.com/pybamm-team/PyBaMM/pull/2823) |
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 you move the comment further up (we follow a decreasing PR number sorting)? I realise some of the existing lines do not follow the order actually, so if you can sort them that would be great hahah.
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.
Sure, will do!
@all-contributors add @jsbrittain for code and tests |
I've put up a pull request to add @jsbrittain! 🎉 |
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 @jsbrittain. The one comment I had was the default of "unbounded" for number of stored integrators. What is the effect of having a finite bound by default (e.g. 100?), it feels like it would be nice to have memory consumption bounded by default.
The default is unbound as this mimics the original code behaviour, but happy to apply an (arbitrary) bound, such as 100, if preferred. |
Thanks @jsbrittain, happy to merge. |
Description
Fix for excessive RAM consumption when running back-to-back simulations. As outlined in #1442, three problems were identified:
solution.last_state
to Simulation.solve() (but requires resolution of [Bug]: Solutionlast_state
can't be used as a starting solution #2788).Fixes #1442 (Excessive RAM consumption when running multiple simulations)
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
(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
$ python run-tests.py --doctest
You can run unit and doctests together at once, using
$ python run-tests.py --quick
.Further checks: