Skip to content

fix: run experiments on non-battery models, raise if steps are ignored - #6014

Open
brosaplanella wants to merge 3 commits into
mainfrom
fix/experiment-non-battery-models
Open

brosaplanella wants to merge 3 commits into
mainfrom
fix/experiment-non-battery-models

Conversation

@brosaplanella

@brosaplanella brosaplanella commented Oct 31, 2026 •

Copy link
Copy Markdown
Member

BaseModel gains the summary_variables property, so the cycle summary of a model that is not a battery model is empty rather than an AttributeError. Simulation raises a ModelError, naming the steps, when the model does not read "Current function [A]", rather than silently ignoring the experiment.

Description

pybamm.Simulation accepts an experiment with any pybamm.BaseModel, but on a model that is not a battery model:

  1. solving failed at the end of the first cycle, because the cycle summary reads model.summary_variables, which only BaseBatteryModel defined;
  2. once that was worked around, the steps had no effect on a model that does not read "Current function [A]", with no error or warning.

Fixes #6013

Changes

  • summary_variables (getter and validating setter) moves from BaseBatteryModel to pybamm.BaseModel, where _summary_variables = [] was already initialised. A model without summary variables gets an empty cycle summary.
  • Simulation raises a ModelError when the model does not read "Current function [A]" as a parameter, naming the steps. Every step sets it, directly (explicit steps) or by replacing it with the current variable (implicit steps).

Steps that drive other parameters are a follow-up (#6014).

Tests

  • TestBaseModel.test_summary_variables_set_get
  • TestSimulationExperiment.test_experiment_on_model_without_summary_variables: a non-battery model that reads the current runs an experiment and has an empty summary.
  • TestSimulationExperiment.test_experiment_on_model_that_does_not_read_the_current

Type of change

Bug fix

Important checks:

Please confirm the following before marking the PR as ready for review:

  • No style issues: nox -s pre-commit
  • All tests pass: nox -s tests
  • The documentation builds: nox -s doctests
  • Code is commented for hard-to-understand areas
  • Tests added that prove fix is effective or that feature works
  • If you are adding or changing a model in the zoo, you have read the policy.

brosaplanella and others added 2 commits October 31, 2026 15:24
BaseModel gains the summary_variables property, so the cycle summary of a
model that is not a battery model is empty rather than an AttributeError.
Simulation raises a ModelError, naming the steps, when the model does not
read "Current function [A]", rather than silently ignoring the experiment.

Co-Authored-By: Claude Opus 5.5 <[email protected]>
@codecov

codecov Bot commented Oct 31, 2026 •

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 98.22%. Comparing base (8e54b57) to head (c8220ca).

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #6014      +/-   ##
==========================================
- Coverage   98.23%   98.22%   -0.01%     
==========================================
  Files         350      350              
  Lines       36173    36181       +8     
==========================================
+ Hits        35533    35540       +7     
- Misses        640      641       +1     

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@brosaplanella
brosaplanella marked this pull request as ready for review October 31, 2026 16:33
@brosaplanella
brosaplanella requested a review from a team as a code owner October 31, 2026 16:34
@github-actions

Copy link
Copy Markdown
Contributor

🐰 Bencher Report

ProjectPyBaMM
Branchfix/experiment-non-battery-models
Testbedbare-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

@brosaplanella

Copy link
Copy Markdown
Member Author

The bencher regression seems to be for SPMe, only so I think it is a one-off.

This branch has not been deployed

No deployments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[Bug]: Experiments fail on, then silently ignore, models that are not battery models

1 participant