Skip to content

Fix ParameterValues.to_json for create_from_bpx partials#5641

Open
rtimms wants to merge 2 commits into
mainfrom
fix/bpx-partial-serialisation
Open

Fix ParameterValues.to_json for create_from_bpx partials#5641
rtimms wants to merge 2 commits into
mainfrom
fix/bpx-partial-serialisation

Conversation

@rtimms

@rtimms rtimms commented Jul 7, 2026

Copy link
Copy Markdown
Contributor

Description

pybamm.ParameterValues.create_from_bpx(...).to_json() raised TypeError: ... got multiple values for argument 'D_ref' (and similar for OCP interpolants / exchange-current density).

create_from_bpx stores several functional parameters as functools.partial objects with keyword arguments already bound, e.g.:

functools.partial(_diffusivity, D_ref=..., Ea=..., constant=True)  # def _diffusivity(sto, T, D_ref, Ea, constant=False)
functools.partial(_interpolant_func, name=..., x=..., y=...)       # OCP interpolants

In convert_function_to_symbolic_expression, the partial branch built its argument list from the full partial signature — which still lists the bound and defaulted parameters (D_ref, Ea, constant) — then called the partial positionally for every one of them. The already-bound kwargs then collided with the positional values.

Fix

Only create symbolic Parameters for the parameters that still lack a value (no default and not variadic) — the genuine symbolic inputs (sto, T, …). Bound kwargs and defaulted control flags (e.g. constant, which is used for Python-level if branching, not as a symbol) keep their values.

Note the subtlety: filtering only on func.keywords is insufficient — a defaulted-but-unbound parameter (constant in the callable-diffusivity case) would still be turned into a symbol and shift positionally into a bound parameter's slot, re-triggering the same collision.

Also removes the dead get_name/get_args branch, which required an object exposing get_name(), get_args(), and .func simultaneously — a type that has never existed in the codebase or its history.

Tests

  • TestConvertFunctionToSymbolicExpression in test_serialisation.py — kwarg-bound partials returning scalars and interpolants, plus the defaulted-unbound-arg regression.
  • Parametrized test_bpx_to_json_roundtrip (function / constant / table forms) in test_bpx.py — full create_from_bpxto_jsonfrom_json round-trip.

🤖 Generated with Claude Code

create_from_bpx stores functional parameters as functools.partial
objects with keyword arguments already bound (e.g. diffusivity binding
D_ref/Ea/constant, OCP interpolants binding name/x/y). The partial branch
of convert_function_to_symbolic_expression built its argument list from
the full partial signature, which still lists those bound and defaulted
parameters, and then called the partial positionally for every one of
them, raising "got multiple values for argument 'D_ref'".

Only create symbols for the parameters that still lack a value (no default
and not variadic) — the true symbolic inputs. Bound kwargs and defaulted
control flags (e.g. `constant`) keep their values. Also drop the dead
get_name/get_args branch, which required an object type that never existed.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
@rtimms rtimms requested a review from a team as a code owner July 7, 2026 09:51
- Pass the traced symbols by keyword rather than positionally, so a partial
  whose bound argument is not the last positional parameter keeps its bound
  value instead of being displaced (robustness beyond the current bpx inputs).
- Remove the redundant func_to_eval alias; call func directly.
- Trim the fallback comment to the two-line convention.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
@codecov

codecov Bot commented Jul 7, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 98.11%. Comparing base (e929c3c) to head (5527ae7).

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #5641      +/-   ##
==========================================
+ Coverage   98.10%   98.11%   +0.01%     
==========================================
  Files         339      339              
  Lines       32069    32061       -8     
==========================================
- Hits        31460    31457       -3     
+ Misses        609      604       -5     

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

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.

1 participant