-
-
Notifications
You must be signed in to change notification settings - Fork 617
Added an option for multiple initial conditions in IDAKLU solver #4981
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?
Added an option for multiple initial conditions in IDAKLU solver #4981
Conversation
CHANGELOG.md
Outdated
- Added "use lumped thermal capacity" option in lumped thermal model ([#4968](https://github.com/pybamm-team/PyBaMM/pull/4968)) | ||
- Added an option for multiple initial conditions in IDAKLU solver ([#4981](https://github.com/pybamm-team/PyBaMM/pull/4981)) |
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.
- Added "use lumped thermal capacity" option in lumped thermal model ([#4968](https://github.com/pybamm-team/PyBaMM/pull/4968))
- Added an option for multiple initial conditions in IDAKLU solver ([#4981](https://github.com/pybamm-team/PyBaMM/pull/4981))
Wrong section, this needs to go into unreleased.
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 also fixed the lumped thermal capacity option in the changelog for the last bugfix release, so just move the new one up to unreleased
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 for the review moved them
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## develop #4981 +/- ##
===========================================
- Coverage 98.57% 98.57% -0.01%
===========================================
Files 304 304
Lines 23652 23687 +35
===========================================
+ Hits 23316 23350 +34
- Misses 336 337 +1 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
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 @Rishab87, this is a great start and on the right track. I've made a few comments and suggestions below, happy to discuss any of them just reply to this or message me on the slack.
src/pybamm/solvers/idaklu_solver.py
Outdated
y0_list = [] | ||
|
||
for ic in initial_conditions: | ||
model_copy = model.new_copy() |
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.
you don't want to copy the model for each initial_condition, as models are quite big
@@ -739,7 +740,11 @@ def solve( | |||
t_interp : None, list or ndarray, optional | |||
The times (in seconds) at which to interpolate the solution. Defaults to None. | |||
Only valid for solvers that support intra-solve interpolation (`IDAKLUSolver`). | |||
initial_conditions : dict, numpy.ndarray, or list, optional |
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.
put somewhere here that this is only for the idaklu solver
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.
and jax perhaps?
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.
yeah right,added that
@@ -911,6 +916,7 @@ def solve( | |||
t_eval[start_index:end_index], | |||
model_inputs_list, | |||
t_interp, | |||
initial_conditions, |
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.
does this arg work for the jax solver?
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.
Not now , I'll add support for jax in a seperate PR in future otherwise this would become too long for now
src/pybamm/solvers/idaklu_solver.py
Outdated
@@ -822,7 +822,44 @@ def supports_parallel_solve(self): | |||
def requires_explicit_sensitivities(self): | |||
return False | |||
|
|||
def _integrate(self, model, t_eval, inputs_list=None, t_interp=None): | |||
def _apply_initial_conditions(self, model, initial_conditions): |
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 function overlaps a bit with BaseModel.set_initial_conditions_from
, have you seen this function?
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.
yeah I saw it but it works only when we have a solution object. In multiple IC scenario, we need to spin up copies of the discretised and built model before we call the IDA integrator so I think we should have a separate _apply_initial_conditions
function?
They serve different purpose also, they live in different layers and they use different data to work with so I think they should be kept separate
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.
happy to keep them separate, just wanted to point it out as this function does a lot of special handling for concatenated symbols which you might need for your _apply_initial_conditions
(or might not?)
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.
by the time we get to _apply_solver_initial_conditions
, the model has already been discretized and flattened into a single y0 vector, so I dont think we need special handling for concatenated symbols
n_sims = 3 | ||
initial_conditions = [{"u": i + 1} for i in range(n_sims)] | ||
inputs = [{} for _ in range(n_sims)] | ||
t_eval = np.linspace(0, 1, 10) |
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.
use:
t_eval = [0, 1]
t_interp = np.linspace(0, 1, 10)
then
solutions = solver.solve(
model, t_eval, t_interp=t_interp, inputs=inputs, initial_conditions=initial_conditions
)
disc = pybamm.Discretisation() | ||
disc.process_model(model) | ||
|
||
solver = pybamm.IDAKLUSolver(rtol=1e-8, atol=1e-10, options={"num_threads": 1}) |
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 you need these tolerances for such a simple problem, just use the defaults
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.
without these getting assertion errors for these problems, alternatively we can add these tolerances while comparing if not here?
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.
when comparing the solution? I would just use the default tolerances, then use these also (perhaps multipled by 10 or so) for your comparison. I notice you don't specify an absolute tolerance for your comparison, the default is 0 which is not suitable for a solution tending to 0.
def _integrate(self, model, t_eval, inputs=None, t_interp=None): | ||
def _integrate( | ||
self, model, t_eval, inputs=None, t_interp=None, intial_conditions=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.
If you are not planning on supporting the jax solver, you need to raise an error if initial_conditions
is not None
): | ||
solver.solve( | ||
model, t_eval, inputs=inputs, initial_conditions=initial_conditions | ||
) |
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.
also need to test for pde models that need to be discretised
@@ -208,3 +208,39 @@ def test_with_experiments(self): | |||
sols[0].cycles[-1]["Current [A]"].data, | |||
sols[1].cycles[-1]["Current [A]"].data, | |||
) | |||
|
|||
def test_multiple_initial_conditions(self): |
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 is just the same as the unit test I think?
for the integration tests, you should use the inbuilt models (at least SPM and DFN) and make sure the feature works with these.
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.
added them
…multiple-initial-conditions
@martinjrobins Thanks for the review, I've made few changes accordingly and replied to all the comments |
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 @Rishab87, test_multiple_initial_conditions_spm
and test_multiple_initial_conditions_dfn
look very similar, can we combine this into a single test that is parameterised by the model class? Also, it would be good to check the solution against two separate solves using those initial conditions, so we know the solution is correct, not just the initial condition
CHANGELOG.md
Outdated
@@ -25,6 +25,8 @@ | |||
## Features | |||
|
|||
- Added "use lumped thermal capacity" option in lumped thermal model ([#4968](https://github.com/pybamm-team/PyBaMM/pull/4968)) | |||
- Added an option for multiple initial conditions in IDAKLU solver ([#4981](https://github.com/pybamm-team/PyBaMM/pull/4981)) |
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.
Move this to unreleased above
@martinjrobins thanks for the review again, I've made few changes in the tests and replied to the comments |
Description
Added an option for multiple initial conditions in IDAKLU solver
Fixes #3713
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-commit
nox -s tests
nox -s doctests