Added an option for multiple initial conditions in IDAKLU solver#4981
Added an option for multiple initial conditions in IDAKLU solver#4981martinjrobins merged 22 commits intopybamm-team:developfrom
Conversation
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## develop #4981 +/- ##
========================================
Coverage 98.88% 98.88%
========================================
Files 320 320
Lines 26949 26981 +32
========================================
+ Hits 26648 26680 +32
Misses 301 301 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
martinjrobins
left a comment
martinjrobins
left a comment
There was a problem hiding this comment.
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.
| 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.
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
)
There was a problem hiding this comment.
This change did not seem to be done yet
There was a problem hiding this comment.
ohh sorry, ig I missed it in few tests, added this change
…multiple-initial-conditions
|
@martinjrobins Thanks for the review, I've made few changes accordingly and replied to all the comments |
martinjrobins
left a comment
martinjrobins
left a comment
There was a problem hiding this comment.
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
|
@martinjrobins thanks for the review again, I've made few changes in the tests and replied to the comments |
martinjrobins
left a comment
martinjrobins
left a comment
There was a problem hiding this comment.
thanks @Rishab87 for making the changes, sorry for the delay in review this fell off my radar. All looks good to me now
|
@martinjrobins thanks for the review |
…ab87/PyBaMM into multiple-initial-conditions
|
@kratman any updates on this? |
Co-authored-by: Eric G. Kratz <kratman@users.noreply.github.com>
|
@Rishab87 From my side we mostly just need traceability on the open items |
|
@kratman I've already addressed open items here:
Anything else you would like to know? |
I think a ticket for the Jax solver portion would be good |
|
@kratman yeah sure I'll keep jax seperate this PR does not have any jax related code, by ticket you mean should I create a seperate issue for jax? |
Yes |
|
Created the issue. |
|
@kratman @martinjrobins any updates on this one? |
martinjrobins
left a comment
martinjrobins
left a comment
There was a problem hiding this comment.
looks all good to me, thanks @Rishab87
3 participants
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-commitnox -s testsnox -s doctests