Repository navigation
fix: processed-variable ordering, 3D unrolling and faster computed reads - #5785
Merged
Merged
Conversation
BradyPlanden
force-pushed
the
a3b/processed-variable-fixes
branch
from
September 23, 2026 19:06
7265f11 to
dc23aad
Compare
|
Check out this pull request on See visual diffs & provide feedback on Jupyter Notebooks. Powered by ReviewNB |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #5785 +/- ##
=======================================
Coverage 98.09% 98.10%
=======================================
Files 346 346
Lines 35407 35458 +51
=======================================
+ Hits 34733 34786 +53
+ Misses 674 672 -2 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
rtimms
reviewed
Sep 24, 2026
rtimms
reviewed
Sep 25, 2026
Contributor
|
Bencher Report| Project | PyBaMM |
| Branch | a3b/processed-variable-fixes |
| 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
a3b/processed-variable-fixes
branch
from
September 25, 2026 12:48
d298cdd to
7e4a8b6
Compare
BradyPlanden
force-pushed
the
a3b/processed-variable-fixes
branch
from
September 25, 2026 13:29
7e4a8b6 to
dec5403
Compare
rtimms
reviewed
Sep 25, 2026
rtimms
left a comment
rtimms
left a comment
Contributor
There was a problem hiding this comment.
The fixes look correct: unsort only on the Hermite route, time-major 3D unrolling, joins at a shared boundary, and the lazy xarray. I checked the np.interp fast path against xarray on a real SPM output_variables solve: values, shape and dtype were bit-identical for endpoints, NaN outside the range, int/float32/list t, unsorted and duplicate query times, and scalar t. A few minor points are inline, plus:
- The new errors are bare
NotImplementedErrors: the spatial time integral inas_computed(), and_raise_if_unjoinable. AGENTS.md prefers PyBaMM exceptions for framework errors. - The mixed-inputs test only checks
all_inputs_stacked. It would be worth also reading a variable end to end, since thatValueErrorwas the user-visible bug. - The bpx notebook's output line looks hand-edited rather than regenerated.
BradyPlanden
force-pushed
the
a3b/processed-variable-fixes
branch
2 times, most recently
from
October 2, 2026 13:04
74632be to
aa5ec02
Compare
martinjrobins
previously approved these changes
Oct 7, 2026
rtimms
reviewed
Oct 8, 2026
rtimms
left a comment
rtimms
left a comment
Contributor
There was a problem hiding this comment.
Reviewed the stack (#5785 → #5794 → #5786). The fixes here look correct to me: I traced both 3D unroll orders, the as_computed() F-order reshape, the unsorted-t fix, the boundary-dropping join and the np.interp fast path, and the tests cover each of them well. One compatibility issue and some minor points are inline.
Two more things:
- The jelly-roll notebook solver swap and the
test_parameter_values_serialisation.pyedit (both from a7c468b) look outside this PR's scope. Worth splitting them out or mentioning them in the description. - The stack needs a rebase onto
main; the only conflict isCHANGELOG.md.
BradyPlanden
force-pushed
the
a3b/processed-variable-fixes
branch
from
October 8, 2026 12:36
79c475d to
45bb795
Compare
martinjrobins
approved these changes
Oct 8, 2026
Return unsorted non-Hermite time queries in query order, unroll computed 3D variables time-major, and stack mixed scalar and vector inputs. Build computed variables' xarray lazily, read 0D ones with np.interp; add Solution.sensitivity_names.
as_computed() flattened each time point in C order, the reverse of the F-ordered CasADi output that unroll_2D/unroll_3D expect, so 2D and 3D variables round-tripped with their spatial axes reordered. It now flattens the kernel's raw observation, before per-class reordering. initialise_3D_scikit_fem read a 2+1D model's current-collector nodes y-fastest, but the mesh numbers them z-fastest, so x-y-z variables from output_variables had y and z swapped. The as_computed tests now use spatially varying data, which the constant data could not catch.
as_computed() reshaped a time integral's entries as if they kept a time axis. A 0D integral is already summed to one value, so it raised a ValueError; scikit-fem integrals are already reordered, so 2D and 3D ones came back scrambled. A 0D integral now passes its summed value with time_indep=True, as an output_variables solve stores it, and spatial time integrals raise NotImplementedError. The 3D-only output_variables test becomes a matrix over every layout ProcessedVariableComputed supports and every time-integral method, each checked against an output_variables solve. A guard fails when a new ProcessedVariable subclass or time-integral method has no case.
Solution.__add__ drops the second solution's first time point when it repeats the first's last, but ProcessedVariableComputed._concat kept that sample's data and sensitivities, so the joined variable held one more value than time points and raised on read. The join now drops them too, and takes the joined variable's times from its segments, because Solution.from_sub_solutions passes the final solution to every join. A time integral from output_variables or as_computed() lost its time independence in the join and could not be read. The variable now carries its ProcessedVariableTimeIntegral: an ExplicitTimeIntegral joins into its integral over both segments, counting the initial condition once. A DiscreteTimeSum, or an expression of a time integral, has no value over the join, since every segment samples its end points; the join still succeeds, and reading the variable raises NotImplementedError. The output_variables matrix now joins every layout and time-integral method through Solution.__add__ at a shared boundary, checked against a full-state join. New tests cover sensitivities at a shared boundary, __add__ and from_sub_solutions over three segments, and an experiment with a DiscreteTimeSum output variable.
ProcessedVariableComputed._concat sums each segment's own integral, so nothing is integrated between segments that do not share a boundary. The docstring now says so, and a test covers the gap for a join of two output_variables solves and of a full-state solve with one.
The all_inputs_stacked test only checked the stacked array. It now also reads a variable from an IDAKLUSolver solve with a scalar and a vector input, which raised a ValueError before the stacking fix.
The stored output of parameter_values.search("Negative electrode") had
one line edited by hand to the new diffusivity name, but that name no
longer matches the search, and the output predates the current search
format. It is now the output of running the cell.
The comment said every query the fast path takes keeps xarray's errors, but an empty or tuple t, which xarray rejected, now returns values.
Solution.all_inputs_stacked re-implemented base_solver's flatten_inputs, which handles mixed scalar and vector inputs and an empty dict the same way. It now uses the helper. solution.py now imports base_solver while pybamm is still initialising, so base_solver defers its annotations; on Python < 3.14 its pybamm.Solution annotation otherwise fails at class creation.
Each changelog entry is now one line, leaving the detail to the PR, and the performance entry moves from Features to Optimizations. Four two-line comments become one line, and a test comment that restated the test's name is removed.
A full-state join integrates an ExplicitTimeIntegral across a gap between the solutions, while a join involving output_variables sums each solution's integral. Solution.__add__ now says so.
_unjoinable, _xr_interp_args and time_integral were set only in __init__, so a ProcessedVariableComputed loaded from a v26.9 pickle raised AttributeError on its first read or join. Class-level defaults cover it; a new test strips them from a pickled variable, before and after its first read.
BradyPlanden
force-pushed
the
a3b/processed-variable-fixes
branch
from
October 8, 2026 15:22
45bb795 to
2beac29
Compare
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
Fixes five bugs that made solutions return wrong values or fail to read, mostly from
output_variablessolves:output_variablesresults were scrambled, by up to 53%.as_computed()reordered 2D and 3D axes and raised on a 0D time integral.output_variablessolutions at a shared time point broke reads. A joinedDiscreteTimeSum, or an expression of a time integral, now raisesNotImplementedErrorwhen read.ValueError.output_variablesreads also build theirxr.DataArrayonly when they interpolate, so an off-gridsolution["Voltage [V]"](t)on an SPM drops from about 420 µs to 1 µs.Also adds
Solution.sensitivity_namesand moves two notebooks and a test off deprecated names. Split out of #5732, with no Rust dependency.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