Repository navigation
refactor: read solution variables through per-model observer caches - #5786
Merged
Merged
Conversation
BradyPlanden
force-pushed
the
a4/observation-seam
branch
from
September 23, 2026 19:09
b51c7e3 to
d5324bd
Compare
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #5786 +/- ##
==========================================
+ Coverage 98.09% 98.11% +0.01%
==========================================
Files 346 348 +2
Lines 35407 35610 +203
==========================================
+ Hits 34733 34939 +206
+ Misses 674 671 -3 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
BradyPlanden
force-pushed
the
a4/observation-seam
branch
from
September 24, 2026 13:48
48c7baa to
a97b0be
Compare
BradyPlanden
force-pushed
the
a4/observation-seam
branch
from
October 2, 2026 12:52
a97b0be to
a5afbe0
Compare
|
Check out this pull request on See visual diffs & provide feedback on Jupyter Notebooks. Powered by ReviewNB |
BradyPlanden
changed the base branch from
main
to
a3b/processed-variable-followups
October 2, 2026 12:52
BradyPlanden
force-pushed
the
a4/observation-seam
branch
from
October 2, 2026 13:04
a5afbe0 to
1bdfb89
Compare
Contributor
|
Bencher Report| Project | PyBaMM |
| Branch | a4/observation-seam |
| Testbed | bare-metal |
🐰 View full continuous benchmarking report in Bencher
⚠️ WARNING: Truncated view!The full continuous benchmarking report exceeds the maximum length allowed on this platform.
BradyPlanden
force-pushed
the
a4/observation-seam
branch
from
October 2, 2026 16:16
1bdfb89 to
01f0a91
Compare
BradyPlanden
changed the title
BradyPlanden
force-pushed
the
a4/observation-seam
branch
from
October 5, 2026 10:05
01f0a91 to
405fe19
Compare
BradyPlanden
force-pushed
the
a4/observation-seam
branch
from
October 5, 2026 11:34
405fe19 to
3e30544
Compare
rtimms
reviewed
Oct 8, 2026
rtimms
left a comment
rtimms
left a comment
Contributor
There was a problem hiding this comment.
The refactor matches the code it replaces as far as I can trace: build_variable against _update_variable, CasadiObserver against the old observe/sensitivity paths, and OutputAssembly.attach against the old IDAKLU loop. Caching time integrals per model is safe because ProcessedVariableTimeIntegral holds only model-level data. The deprecation shim and old-pickle loading are tested. Two minor points inline.
BradyPlanden
force-pushed
the
a4/observation-seam
branch
from
October 8, 2026 12:36
3e30544 to
dfc15bc
Compare
BradyPlanden
force-pushed
the
a4/observation-seam
branch
from
October 8, 2026 15:22
dfc15bc to
cb4ea47
Compare
A Solution reads variables through one ObservationBackend and a ProcessedVariable through one VariableObserver (CasADi only); OutputAssembly owns the output_variables layout and keys its sensitivities by sensitivity parameter. Solution carries its backend in an _observation slot, which a solution pickled before it existed loads as CASADI_OBSERVATION.
A Solution carried an observation backend, the module-level CASADI_OBSERVATION, which first_state, last_state, +, from_sub_solutions and copy sliced and joined. The backend held no state, so every slice and join returned it unchanged, while the CasADi leaves it compiled were cached on each model. A Solution now holds no observation state. build_variable reads each segment through its own model's ObserverCache, which replaces model._variables_casadi and keeps its rules: keyed by variable name, copied by new_copy, and skipping time integrals. A model pickled before the cache existed builds one on first read. ObservationBackend, CasadiObservation, CASADI_OBSERVATION and join_observations are removed with their tests.
ObserverCache skipped time integrals: it cached only the CasADi leaf, so a hit lost the integral and would have read its integrand. Every read of a time integral therefore converted it again for each segment, 20 times for a 20-step experiment, and again for every solve of the model. The cache now keeps a leaf with the expression it evaluates and its time integral. The integral's analysis depends only on its expression, and nothing changes it after conversion, so solutions of one model can share it. Reading an ExplicitTimeIntegral from a 20-step experiment drops from 1.9 ms to 0.6 ms on first read and from 1.5 ms to 0.01 ms on later solves of the model, with identical values.
Each changelog entry is now one line, leaving the detail to the PR, and the performance entry moves from Features to Optimizations. Eight two-line comments become one line, and four that restated the code they moved with are removed. VariableObserver holds its leaves rather than owning them: the model's ObserverCache shares them.
The solver returns no rows for such a variable, so its payload is (n_t, 0). Covers it as the only output and alongside a regular one.
BradyPlanden
force-pushed
the
a4/observation-seam
branch
from
October 8, 2026 16:39
cb4ea47 to
2d1f76d
Compare
martinjrobins
approved these changes
Oct 8, 2026
2 of 3 tasks
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description
Reading a variable back from a
Solutionwas spread acrossSolution,ProcessedVariableandIDAKLUSolver, each re-deriving the same facts. This gathers it into two modules, with identical values:solvers/variable_observer.py:VariableObserverandCasadiObserver, moved fromProcessedVariable. An observer serialises its CasADi functions and selects its segments once, so a repeatsolution["Voltage [V]"](t)on an SPMe discharge is about 40% faster.solvers/observation.py:build_variable, which reads each segment through its own model'sObserverCache(replacingmodel._variables_casadi), andOutputAssembly, moved fromIDAKLUSolver. ASolutionholds no observation state.ProcessedVariable'sbase_variables_casadiis deprecated, and solutions pickled by earlier versions still load. The Rust path (#5732) will add a second reader tobuild_variable.Type of change
Add an entry under
# [Unreleased]in CHANGELOG.md, in one of## Breaking changes,## Deprecated,## Features, or## Bug fixes(include PR number; breaking and deprecation entries need a one-line migration note). Internal-only PRs — refactor, docs, CI, tests — can skip this. See RELEASE.md for the full policy.Important checks:
Please confirm the following before marking the PR as ready for review:
nox -s pre-commitnox -s testsnox -s doctests