Conversation
There was a problem hiding this comment.
Pull request overview
This PR implements a unified experiment model pathway that reuses a single processed/built model (and solver) across compatible experiment steps, switching the “active” step via one-hot, solve-time inputs. It keeps the legacy per-step build available, with experiment_model_mode controlling selection and fallback behavior.
Changes:
- Added
experiment_model_mode={"auto","unified","legacy"}(with"per-step"alias) toSimulation, plus unified-model construction, step-weight inputs, and unified termination decoding. - Extended experiment step APIs to expose symbolic helpers for control residuals and combined termination expressions, enabling experiment-wide weighted expressions.
- Added extensive unit coverage and a benchmark script comparing unified vs legacy experiment performance.
Reviewed changes
Copilot reviewed 10 out of 10 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
src/pybamm/simulation.py |
Core unified experiment-model build path, one-hot inputs, solver/model reuse, termination decoding, and mode selection/fallback logic |
src/pybamm/experiment/step/base_step.py |
Adds termination-expression helpers and refactors control-submodel wiring for reuse by unified experiments |
src/pybamm/experiment/step/steps.py |
Adds get_control_residual implementations for implicit steps and custom-step support constraints |
src/pybamm/experiment/step/step_termination.py |
Refactors terminations to expose event name + symbolic expression, retaining get_event construction |
src/pybamm/models/submodels/external_circuit/function_control_external_circuit.py |
Updates resistance control residual form and adds ExperimentFunctionControl for weighted control residuals |
src/pybamm/models/submodels/external_circuit/__init__.py |
Exposes ExperimentFunctionControl in the external circuit submodule exports |
tests/unit/test_experiments/test_simulation_with_experiment.py |
Adds/updates tests validating unified vs legacy behavior, fallback/rejection cases, and result equivalence |
tests/unit/test_experiments/test_experiment_steps.py |
Tests new symbolic helper methods for terminations and control residuals |
scripts/benchmark_unified_experiment.py |
New benchmark driver for unified vs legacy experiment execution |
CHANGELOG.md |
Documents the new unified experiment-model path feature |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #5422 +/- ##
==========================================
+ Coverage 98.24% 98.31% +0.06%
==========================================
Files 330 331 +1
Lines 30072 30471 +399
==========================================
+ Hits 29544 29957 +413
+ Misses 528 514 -14 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
|
Check out this pull request on See visual diffs & provide feedback on Jupyter Notebooks. Powered by ReviewNB |
MarcBerliner
left a comment
There was a problem hiding this comment.
Thanks Martin, the build time improvements are a nice win. I left some comments on a few areas I think we should sort out before this is ready to merge
Raw medians
Unified vs legacy
|
Description
fixes #5407
This PR adds a unified experiment-model path that reuses one processed model and one solver across compatible experiment steps, instead of building one model per step.
The unified path combines per-step control and termination behavior into shared experiment-wide expressions, and switches the active step through solve-time one-hot inputs.
For step weights$w_i$ , step control residuals $g_i(x)$ , and step-local termination aggregates $h_i(x)$ , the unified model uses:
and
where exactly one$w_i$ is active for each solve step.
UPDATE: the sum over one-hot weights has been replaced by a conditional expression over a single "step index" input, see discussion below
Examples of the control residuals include:
For steps with multiple terminations$f_{ij}(x)$ , the step-local aggregate is formed as a nested minimum so that any active termination can stop the step:
The legacy per-step build remains available, and
Simulationnow supports:experiment_model_mode="unified"experiment_model_mode="legacy"(default)Unsupported cases raise clearly in
unifiedmode.Benchmarks
Benchmark script:
scripts/benchmark_unified_experiment.pyScenarios:
event_driven_cccvCharge at C/3 until 4.1 VHold at 4.1 V until C/20mixed_controlDischarge at C/20 for 20 minutesCharge at 1 A for 10 minutesHold at 4.1 V for 10 minutesDischarge at 2 W for 10 minutesDischarge at 4 Ohm for 10 minutesstart_time_paddingCurrent(1, duration=20 minutes, start_time=2023-01-01 08:00:00)Rest(duration=20 minutes, start_time=2023-01-01 09:00:00)Models:
SPMDFNThe script was run on both this branch and
main.mainlegacy timings were effectively the same as this branch’s legacy timings, so the comparison is consistent.Current branch results
mainlegacy comparisonmainlegacy (s)On this machine, the unified path is faster in all benchmarked cases, with the largest gains in mixed-control and start-time experiments.