Repository navigation
fix: validate solver inputs, options and model formats - #5784
Merged
Merged
Conversation
Validate and serialise model.convert_to_format, reject unknown IDAKLU options and sensitivity restarts from output-only solves, and share the input-stacking helpers across solvers. Also run nproc=1 multi-input solves in process and fix the per-state root-solver atol, the AlgebraicSolver minimize gradient and the format-None event check.
BradyPlanden
force-pushed
the
a3/solver-inputs-options
branch
from
September 23, 2026 19:02
a54e604 to
92dc5d7
Compare
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #5784 +/- ##
=======================================
Coverage 98.00% 98.00%
=======================================
Files 346 346
Lines 35259 35300 +41
=======================================
+ Hits 34554 34595 +41
Misses 705 705 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
Napoleon rendered the class docstring's Attributes entry as a second description of the new property, which fails the -W docs build.
martinjrobins
requested changes
Sep 25, 2026
martinjrobins
left a comment
Contributor
There was a problem hiding this comment.
looks great, but I don't think convert_to_format = None is valid? this is just the same as , say, convert_to_format = 'asdf', which you are erroring on.
# Conflicts: # CHANGELOG.md # packages/pybamm/src/pybamm/solvers/idaklu_solver.py
None has evaluated as "python" since 2022, not by keeping the expression tree as documented. Setting it now raises a DeprecationWarning and keeps its current behaviour until removal.
Contributor
|
| Project | PyBaMM |
| Branch | a3/solver-inputs-options |
| Testbed | bare-metal |
⚠️ WARNING: Truncated view!The full continuous benchmarking report exceeds the maximum length allowed on this platform.
🚨 1 Alert
🐰 View full continuous benchmarking report in Bencher
martinjrobins
approved these changes
Sep 29, 2026
BradyPlanden
enabled auto-merge (squash)
September 29, 2026 08:12
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
Several solver inputs could be wrong without anyone noticing:
model.convert_to_formatwas a plain attribute, so a typo was accepted and surfaced at solve time, if at all.convert_to_format, so a model saved as"python"came back as"casadi".IDAKLUSolverandNonlinearSolvermerged unknownoptionskeys through, so{"num_thread": 4}silently kept the defaults.Consolidating that code turned up several bugs. Restarting a sensitivity solve from an
output_variablessolve seeded the state sensitivities from the output sensitivities: repeatedstepcalls failed with an unrelated shape error or returned wrong gradients, and an SPM discharge-then-restExperimentreturned a voltage sensitivity 19% below finite differences. Several input sets withnproc=1went through a one-processmultiprocessingpool, which ran the same solves in the same order as a loop but spawned an interpreter and pickled the model and every solution. For a 32-set DFN withcalc_ic=False, finding consistent initial conditions took 1.8 s instead of 3 ms, and on macOS and Windows an unguarded script hung. There were also three smaller bugs: a per-stateatolarray crashed the root-solver setup,AlgebraicSolver's"minimize"gradient was wrong for non-CasADi formats, and the initial event check raisedUnboundLocalErrorforconvert_to_format=None.Models and simulations pickled by earlier versions still load, and JSON files written by earlier versions load with the model's default format.
BaseModel.convert_to_formatbecomes a validated property that raisesOptionError, with a__setstate__migration for old pickles and auses_stacked_inputsflag that names the input convention rather than the format.convert_to_format.stack_inputs/flatten_inputshelpers inbase_solver.pyreplace the per-solver copies.BaseSolver._overlay_optionsrejects unknown option keys, andIDAKLUSolverandNonlinearSolveruse it.BaseSolver._integrateloops in the calling process whennproc == 1.step()rejects a sensitivity restart from an outputs-only solution with aSolverError. In anExperimentthis ends the experiment after its first step, as other step failures do.atol, the"minimize"gradient and theNone-format event check.This is one of the backend-neutral pre-flight PRs split out of the Rust execution engine (#5732). It has 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