Fix deprecated "electrode diffusivity" alias silently clobbering "particle diffusivity"#5642
Open
rtimms wants to merge 5 commits into
Open
Fix deprecated "electrode diffusivity" alias silently clobbering "particle diffusivity"#5642rtimms wants to merge 5 commits into
rtimms wants to merge 5 commits into
Conversation
…ticle diffusivity" `ParameterValues.check_parameter_values` migrated the deprecated "<X> electrode diffusivity [m2.s-1]" name to the current "<X> particle diffusivity [m2.s-1]" by adding the new key without removing the deprecated one and by unconditionally overwriting the current name. As a result a parameter set could hold both names, and any re-normalisation (copying, `update`, serialisation round-trips) let the stale deprecated value overwrite a value the user set on the current name — silently no-opping diffusivity fits. `create_from_bpx` made this the default by emitting both. - `check_parameter_values`: the deprecated diffusivity name now migrates with the current name taking precedence (`setdefault`) and the deprecated key removed, so the two can never coexist. The `FuzzyDict` lookup fallback keeps reads of the old name working. - `bpx.py`: emit the current "particle diffusivity" (and its activation energy) name directly, so `create_from_bpx` yields a clean single-name set with no spurious deprecation warning. - Added regression tests for the migration and the BPX output. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #5642 +/- ##
==========================================
- Coverage 98.10% 98.09% -0.01%
==========================================
Files 339 339
Lines 32069 32075 +6
==========================================
+ Hits 31460 31465 +5
- Misses 609 610 +1 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
Per review, do not drop the deprecated '<X> electrode diffusivity [m2.s-1]' key — custom models may still reference it, and the FuzzyDict fallback already resolves reads. Fixing the clobber only needs the current name to take precedence (setdefault), not deletion. Also warn when both names are set, and leave the MSMR migration branch unchanged. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
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
Setting or fitting
"<phase> particle diffusivity [m2.s-1]"could have zero effect on the solved result: perturbing it changed nothing, because the deprecated"<phase> electrode diffusivity [m2.s-1]"alias silently overrode it. This bites hardest with BPX, sincecreate_from_bpxproduced a set containing both names for the same quantity.Root cause
ParameterValues.check_parameter_valuesmigrated the deprecated name to the current one by adding the new key without removing the deprecated one, and by unconditionally overwriting the current name:So a set could hold both names, and any re-normalisation (
ParameterValues(dict(pv)),.update(...), serialisation round-trips common in fit harnesses) let the stale deprecated value overwrite a value the user set on the current name.Fix
check_parameter_values: the deprecated diffusivity name now migrates with the current name taking precedence (setdefault) and the deprecated key removed, so the two can never coexist and clobber each other. TheFuzzyDictlookup fallback is unchanged, so reads of the old name still resolve.bpx.py: emit the currentparticle diffusivityname (and its activation energy) directly, socreate_from_bpxyields a clean single-name set with no spuriousDeprecationWarning.Type of change
Verification
ParameterValues({"Negative electrode diffusivity [m2.s-1]": D}),keys()contains only theparticlename; settingparticlethen round-trippingParameterValues(dict(pv))preserves the set value (no clobber).create_from_bpxoutput contains onlyparticle diffusivitynames and emits no diffusivity deprecation warning.test_bpx.py,test_parameter_values.py,test_util.py,test_parameter_values_serialisation.py.🤖 Generated with Claude Code