Skip to content

Fix BPX 1.1 hysteresis parameter mapping#5507

Draft
rtimms wants to merge 4 commits into
pybamm-team:mainfrom
rtimms:fix/bpx-hysteresis-mapping
Draft

Fix BPX 1.1 hysteresis parameter mapping#5507
rtimms wants to merge 4 commits into
pybamm-team:mainfrom
rtimms:fix/bpx-hysteresis-mapping

Conversation

@rtimms
Copy link
Copy Markdown
Contributor

@rtimms rtimms commented May 11, 2026

Summary

Follow-up to #5469 (which added BPX 1.x support). The newly-introduced hysteresis fields on BPX 1.1's Particle block weren't being translated by parameters/bpx.py::_get_pybamm_name, so:

  • OCP (lithiation) [V] ended up as Negative electrode OCP (lithiation) [V] (PyBaMM expects Negative electrode lithiation OCP [V])
  • OCP (delithiation) [V] similarly
  • OCP hysteresis decay constant ended up under its raw BPX alias on the electrode pre-name; PyBaMM's Axen one-state hysteresis OCP submodel (OneStateHysteresisOpenCircuitPotential) evaluates two FunctionParameters — <Domain> particle lithiation hysteresis decay rate and <Domain> particle delithiation hysteresis decay rate — both on the particle pre-name. The BPX scalar now fans out to both with the same value.

While auditing the State section I found three adjacent gaps and fixed them in the same PR:

  • Initial hysteresis state: Positive/Negative electrode were extracted into bpx.state but never copied to the PyBaMM dict, so the FunctionParameter was always falling back to 0. Now mapped to [<Phase>: ]Initial hysteresis state in <domain> electrode. Handles both the scalar (single-phase) and the per-particle-phase dict (blended) shapes by routing the blended-ness decision through the existing _get_phase_names helper and matching BPX phase keys to PyBaMM Primary: / Secondary: by particle declaration order.
  • Total heat transfer coefficient [W.m-2.K-1] from bpx.state.thermal_environment was being silently clobbered to 0 a few lines later by an unconditional pybamm_dict.update(...). Replaced with setdefault.
  • target_soc defaulted to 1.0 regardless of what the BPX file declared in State.initial_conditions.initial_soc. The default is now None, resolved to the BPX value when a State section is present and 1.0 otherwise.

Implementation note: _get_pybamm_name now returns list[str] instead of str to support the 1→2 fan-out for the decay constant. The two call sites in _bpx_to_domain_param_dict iterate the returned list. Missing/None-valued optional hysteresis fields are skipped so they don't pollute the parameter dict.

Refs #5463.

Test plan

  • All existing BPX tests in tests/unit/test_parameters/test_bpx.py still pass (13/13)
  • New unit tests (8) added to the same file:
    • Single-phase BPX hysteresis fields → expected PyBaMM names; legacy names not present
    • Missing hysteresis fields are skipped, not added as None
    • Blended-electrode hysteresis fields → phase-prefixed PyBaMM names
    • Initial hysteresis state (scalar form)
    • Initial hysteresis state (per-particle-phase dict form, blended)
    • Heat transfer coefficient regression test (BPX-supplied value preserved)
    • target_soc=None falls back to BPX Initial state-of-charge
    • End-to-end: BPX-derived parameter set runs the Axen one-state hysteresis SPM through a Discharge experiment

Notes for reviewer

  • BPX models only the Axen one-state hysteresis OCP, so the decay constant intentionally does NOT fan out to the Wycisk differential-capacity variant's <Domain> particle hysteresis decay rate (without lithiation/delithiation prefix). Users on that submodel will still need to add it manually.
  • BPX State.degradation (LLI, LAM: Positive electrode, LAM: Negative electrode) is still unhandled — it has no direct PyBaMM equivalent and is out of scope here. Worth a separate issue if/when there's demand.

🤖 Generated with Claude Code

rtimms and others added 2 commits May 11, 2026 14:23
`OCP (lithiation) [V]` / `OCP (delithiation) [V]` / `OCP hysteresis decay
constant` were being passed through `_get_pybamm_name` unchanged, so the
resulting parameter set carried the literal BPX aliases (e.g. "Negative
electrode OCP (lithiation) [V]") instead of the names PyBaMM's Axen
one-state hysteresis OCP submodel evaluates. The decay constant in particular
also needs to fan out to two FunctionParameters (lithiation + delithiation
branches) on the particle pre-name, not the electrode pre-name.

While here:

- Extract `Initial hysteresis state: Positive/Negative electrode` from
  `bpx.state.initial_conditions` (previously silently dropped). Handles both
  scalar values for single-phase electrodes and per-particle-phase dicts for
  blended electrodes, mapping BPX phase names to PyBaMM `Primary: ` /
  `Secondary: ` prefixes by particle order.
- Replace the unconditional `pybamm_dict.update(...)` for `Total heat transfer
  coefficient [W.m-2.K-1]` with `setdefault` so the value pulled from
  `bpx.state.thermal_environment` isn't silently clobbered back to 0.
- Default `target_soc` in `create_from_bpx` / `create_from_bpx_obj` to `None`
  and resolve it to `bpx.state.initial_conditions.initial_soc` when the BPX
  file has a State section. Falls back to 1.0 when there is no State section.

Refs pybamm-team#5463 (follow-up to pybamm-team#5469).

Co-Authored-By: Claude Opus 4.7 (1M context) <[email protected]>
- `bpx_to_param_dict` already builds `domain_phases` at the top; the new
  initial-hysteresis-state loop was recomputing `_get_phase_names` per
  electrode. Reuse the cached value.
- Lift the two `OCP (lithiation/delithiation) [V]` rename branches in
  `_get_pybamm_name` into a `_BPX_OCP_HYSTERESIS_RENAMES` mapping next to
  `_OPTIONAL_HYSTERESIS_FIELDS`.
- Compare `domain.name` rather than `domain is negative_electrode` for the
  decay-constant fan-out, so dispatch doesn't lean on the module-level
  singleton invariant.

Co-Authored-By: Claude Opus 4.7 (1M context) <[email protected]>
@rtimms
Copy link
Copy Markdown
Contributor Author

rtimms commented May 12, 2026

@ejfdickinson could you please take a look and see if any issues remain with any BPX files you have?

@codecov
Copy link
Copy Markdown

codecov Bot commented May 12, 2026

Codecov Report

❌ Patch coverage is 97.72727% with 1 line in your changes missing coverage. Please review.
✅ Project coverage is 98.09%. Comparing base (ce1a240) to head (a7ab0d0).
⚠️ Report is 1 commits behind head on main.

Files with missing lines Patch % Lines
src/pybamm/parameters/bpx.py 97.56% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #5507      +/-   ##
==========================================
- Coverage   98.09%   98.09%   -0.01%     
==========================================
  Files         338      338              
  Lines       31246    31272      +26     
==========================================
+ Hits        30650    30675      +25     
- Misses        596      597       +1     

☔ 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.

@ejfdickinson
Copy link
Copy Markdown

ejfdickinson commented May 13, 2026

@rtimms Thanks, everything BPX-related is working for me for the test cases we have.

Notes:

  1. There is an oddity around initial hysteresis state parameters being created for materials that don't have an attached hysteresis model, but this is inherited from the BPX limitations, right? I assume these parameters don't do anything, and you don't know what model is targeted.

  2. I did one test case (full DFN) with a single-material (blended description) graphite-SiOx electrode but setting
    options={"open-circuit potential": ("one-state hysteresis", "single")}
    This fails at discretisation stage with ModelError: model is overdetermined (extra algebraic keys) from the well-posedness checks. Is this something you're familiar with? It seems like it should be a separate issue but let me know if you'd like a repeat case, if you don't have any similar parameter sets.

@rtimms
Copy link
Copy Markdown
Contributor Author

rtimms commented May 13, 2026

@ejfdickinson thanks for testing!

  1. Yes, exactly. These will just be ignored if they don't show up in the model.
  2. I can't reproduce this. Could you share an example?

@ejfdickinson
Copy link
Copy Markdown

@rtimms I diagnosed the problem - it was due to some placeholder zeroes for electrolyte properties, so no issues here. Just the runtime error path had changed compared to expectations from previous versions.

Good to go for this PR as far as I can see! Thank you!

pybamm.logger.warning(
"Initial concentrations cannot be set using stoichiometry limits for "
"blend electrodes. Please set the initial concentrations manually."
)
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi @rtimms, can we please have the initial concentrations calculated via ESOH and added in for blended electrodes too?

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This would be great, but it's a known limitation also for beginning-of-life preceding the degradation content in BPX, so I'd ask this not be at the expense of delaying a PyBaMM release covering the schema support!

@Daniel-Nicolae23 @rtimms You are welcome to inspect some of the tools we use to implement this after BPX import in our AEPyBaMM toolkit. Currently the code supports this for negative electrode blends only, but we are also working on some general-purpose utilities to be released soon.

https://github.com/About-Energy-OpenSource/AEPyBaMM/blob/main/src/aepybamm/sci_tools.py

see compute_lithiation_bounds and add_initial_concentrations

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.

3 participants