Skip to content

Add a 'State' section#112

Closed
pipliggins wants to merge 7 commits into
FaradayInstitution:mainfrom
pipliggins:4-state-params
Closed

Add a 'State' section#112
pipliggins wants to merge 7 commits into
FaradayInstitution:mainfrom
pipliggins:4-state-params

Conversation

@pipliggins
Copy link
Copy Markdown
Collaborator

Fixes #4

@codecov-commenter
Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 99.09091% with 1 line in your changes missing coverage. Please review.
⚠️ Please upload report for BASE (main@0cf060d). Learn more about missing BASE report.

Files with missing lines Patch % Lines
bpx/schema.py 99.09% 0 Missing and 1 partial ⚠️
Additional details and impacted files
@@           Coverage Diff           @@
##             main     #112   +/-   ##
=======================================
  Coverage        ?   97.01%           
=======================================
  Files           ?        8           
  Lines           ?      436           
  Branches        ?       44           
=======================================
  Hits            ?      423           
  Misses          ?        7           
  Partials        ?        6           

☔ View full report in Codecov by Sentry.
📢 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.

@pipliggins
Copy link
Copy Markdown
Collaborator Author

The current BPX standard doc allows multiple State variables to be expressed with colon-separated keys (e.g. LAM: Positive electrode[: Material]), where the optional : Material extension is used for blended electrodes (with material names matching those listed under the particle field).

This creates an issue in Pydantic, since the material names are not pre-defined and there's an arbitrary number of them. To handle this, each colon-separated key must be treated as “extra” fields rather than being strictly defined. This PR demonstrates that the current standard can be implemented this way, but it requires extensive regex matching and is therefore somewhat fragile - untested material names and dodgy formatting may slip through the validation. More importantly, these colon-separated variables cannot be accessed via dot notation like other variables (since we cannot predefine snake_case names for each one, and the BPX names have spaces in them).

An alternative would be to slightly adapt the standard so it better reflects the current electrode structure. Instead of:

Degradation: {
  "LLI": 5
  "LAM: Positive electrode: Primary": 10,
  "LAM: Positive electrode: Secondary": 20
  "LAM: Negative electrode": 15
}

we could allow each variable: electrode key to be given either as a single float (for single-material electrodes), or as a dict[str, FloatInt] mapping materials to floats:

Degradation: {
  "LLI": 5
  "LAM: Positive electrode": {
     "Primary": 10,
     "Secondary": 20
     }
  "LAM: Negative electrode": 15
}

In this way each 'material' key in the electrode particle dict should have a corresponding key within the relevant state variables.
This would allow regular dot notation/ [key access] to all variables within python. It should also be easier to read and maintain, with less custom validation required.

FYI @rtimms @ejfdickinson @martinjrobins - LMK which route you'd prefer to go with.

@martinjrobins
Copy link
Copy Markdown
Collaborator

This seems like a good idea to me on the software side, as long as we are free to modify the published standard?

@ejfdickinson
Copy link
Copy Markdown
Collaborator

@pipliggins Thank you for this well-reasoned suggestion! For my part I think it would be fine to implement according to your suggested adaptation of the standard, and for @rtimms and I to advocate a matching 'patch' to the written standard at the first opportunity.

@rtimms
Copy link
Copy Markdown
Collaborator

rtimms commented Oct 6, 2025

@pipliggins can this be closed now in favour of #113 ?

@pipliggins
Copy link
Copy Markdown
Collaborator Author

@pipliggins can this be closed now in favour of #113 ?

@rtimms Yes if everyone's happy with the implementation of #113 we can close this.

@pipliggins
Copy link
Copy Markdown
Collaborator Author

Closing as superseded by #113

@pipliggins pipliggins closed this Oct 9, 2025
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.

"State" parameters

5 participants