Repository navigation
feat(idaklu): solver statistics, self-scheduled input-set sweeps and polymorphic Newton - #5782
Merged
Merged
Conversation
BradyPlanden
force-pushed
the
a2/idaklu-stats-scheduling
branch
from
September 23, 2026 18:50
393dad1 to
8ae1649
Compare
BradyPlanden
force-pushed
the
a2/idaklu-stats-scheduling
branch
from
September 23, 2026 18:54
8ae1649 to
1d0e59e
Compare
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #5782 +/- ##
=======================================
Coverage 98.08% 98.09%
=======================================
Files 346 346
Lines 35275 35323 +48
=======================================
+ Hits 34599 34649 +50
+ Misses 676 674 -2 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
martinjrobins
requested changes
Sep 25, 2026
martinjrobins
left a comment
Contributor
There was a problem hiding this comment.
thanks @BradyPlanden, looks great, minor suggestions below and (possible) missing stats on adding two solutions
aabills
added a commit
that referenced
this pull request
Sep 28, 2026
* Release v26.9.0.0 and pybammsolvers v0.10.0 Update CHANGELOG.md and CITATION.cff for v26.9.0.0. Merge the stray second Breaking changes block into the first, drop the citation of the unmerged #5782, and move the #5688 and #5630 entries out of v26.8.0.0, which was tagged before they merged. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> * Cite #5690 for the VTK plotting changelog entry #5689 was closed unmerged; its VTK plotting work landed through #5690. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 5.5 <noreply@anthropic.com>
BradyPlanden
force-pushed
the
a2/idaklu-stats-scheduling
branch
from
September 30, 2026 10:44
5ba9f34 to
00495fe
Compare
Contributor
|
| Project | PyBaMM |
| Branch | a2/idaklu-stats-scheduling |
| 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.
martinjrobins
previously approved these changes
Sep 30, 2026
martinjrobins
left a comment
Contributor
There was a problem hiding this comment.
looks great, thanks @BradyPlanden
Member
Author
|
Found a few regressions on a second pass + incremented pybammsolvers version to avoid a broken pybamm release. Could you re-review @martinjrobins |
…polymorphic Newton Solutions carry IDAKLU's counters as SolverStatistics, summed as solutions combine; input sets are self-scheduled with every failing set named; the standalone Newton owns its Expressions; per-state atol takes lists and tuples. Requires pybammsolvers 0.10.0.
With 16 sets the GIL-free thread could finish all 15 of its sets before thread 0 reached its first step on a loaded macOS runner.
Pass IDAKLUStats by const reference; suppress cppcheck where it cannot see a use (the omp num_threads clause, members used only in the .cpp); mark the tests' pickle round trips nosec, as main does.
Drop the num_threads() comment, which described the change rather than the code, and name what the atomic counter stands in for.
…solutions The __add__ short-circuit, and from_sub_solutions skipping the same segment, dropped that operand's statistics. Its samples add nothing, but the solver may still have worked for it.
A set whose integration fails part-way returns a partial solution with a negative flag instead of throwing, so the per-set post-processing raised on the first such set and hid the rest. Collect them all after the sweep, and when another set throws, name the part-way failures alongside it, so the SolverError and each on_failure="warn" warning carry "input set N: ".
An experiment solution is folded from the cycles it keeps, so the solver work of a cycle that save_at_cycles leaves out went uncounted. Total those cycles' statistics as they are dropped, keeping none of their solutions alive, and add them to the folded solution's.
NumPy upcasts a bool beside numbers, so [True, 1e-6] became a float array that passed the dtype check as a tolerance of 1.0.
The solver statistics, self-scheduled sweep and per-set failure messages need a pybammsolvers newer than the released 0.10.0, so pin pybamm to 0.11.
A 0D domain's mass matrix block is dense, so a model whose differential variables are all scalars or 0D handed block_diag only dense blocks, which SciPy 1.18 deprecates in favour of returning a sparse array. Convert each block to csr_matrix first.
The prebuilt-wheel path resolved pybamm's dependencies afresh with uv pip install --no-sources, so CI tested the latest releases rather than the locked set local nox sessions use. Sync the lock as usual, skipping pybammsolvers, then install this interpreter's wheel with --no-deps.
pytest.warns records every warning raised in its block, not just the matching ones, so an unrelated warning made re.match return None. Build the model outside the block and ignore records that are not a partial-solution warning.
BradyPlanden
force-pushed
the
a2/idaklu-stats-scheduling
branch
from
September 30, 2026 15:53
cb77bfd to
06e7136
Compare
martinjrobins
approved these changes
Sep 30, 2026
rtimms
approved these changes
Oct 1, 2026
Merged
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
IDAKLU already collects SUNDIALS' integrator counters, but only prints them under
print_stats. This PR returns them with every solution from the pybammsolvers binding and exposes them asSolution.solver_statistics, apybamm.SolverStatisticsdataclass (steps, linear-solver setups, nonlinear iterations, nonlinear failures, error-test failures). Counts accumulate acrosst_evalbreakpoints, are kept per input set, and are summed when solutions combine (+, stepping, experiments), so solver work can be asserted in tests and tracked in benchmarks without parsing stdout.A multi-input solve used to split the input sets into equal static blocks per thread, plus a serial remainder on the calling thread. A heterogeneous sweep, such as a current sweep whose high currents stop early on a voltage cut-off, then waited on its slowest block. Sets are now handed out one at a time from a shared counter. The calling thread, which holds the GIL, always takes set 0, so its diagnostics still stream live. A failing set no longer hides the others: the error names every set that failed (
input set 2: ...; input set 5: ...).For pybammsolvers maintainers: pybamm now reads
solution.statsunconditionally, so this bumps pybammsolvers to 0.10.0 and pinspybammsolvers>=0.10.0,<0.11.0.pybammsolvers-v0.10.0has to be released before a pybamm release that contains this PR. #5783 (IDAS sensitivity scales) makes the identical version bump.IDAKLUStatstravels throughSolutionDatatoSolutionand is bound asidaklu.SolverStats. Statistics are saved on every solve, not only underprint_stats. The BBD preconditioner's counter is read only when that preconditioner is in use; withpreconditioner="none"SUNDIALS read it out of the model's own data, which also fixes a garbage value inprint_stats.IDAKLUSolverGroupruns#pragma omp parallel num_threads(team)over astd::atomiccounter, rather thanschedule(dynamic), which the macOS wheels' libomp and MSVC do not support. It still solves every set when the runtime starts fewer threads than asked (OMP_THREAD_LIMIT, nested regions). A Python exception such asKeyboardInterruptstops new sets being taken, while an integrator failure is recorded against its set and the sweep continues.StandaloneAlgebraicSystemowns its residual and Jacobian asunique_ptr<Expression>, with the CasADi constructor delegating to theExpressionone, andExpressiongains a virtual destructor.IDAKLUSolveraccepts a per-stateatolas a list, tuple or array, so it survives ato_config/from_configround trip, and rejects one of the wrong width, booleans and non-real values with aSolverError.SolverStatisticsis documented in the API reference. Solutions pickled before this change still load.This is one of the backend-neutral pre-flight PRs split out of the Rust execution engine (#5732). It has no Rust dependency. It overlaps #5783 (IDAS sensitivity scales) in
IDAKLUSolverGroup.cppand the IDAKLU tests, so whichever lands second needs a rebase.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