Repository: pybamm-team/PyBaMM (python)

Issue:
[Bug]: "electrode diffusivity" -> "particle diffusivity" is still breaking change on `develop` when using pre-24.5 pybamm.ParameterValues
### PyBaMM Version

develop

### Python Version

3.9.13

### Describe the bug

Since #4072 re-inverted the logic fix in #3871, the release candidate on `develop` causes the rename of "electrode diffusivity" -> "particle diffusivity" to be a breaking change. Parameter sets using the old key names do not simulate in the 24.5 RC.

### Steps to Reproduce

```python
import pybamm

parameter_values = pybamm.ParameterValues("Chen2020")

# Dummy a pybamm.ParameterValues object with a legacy parameter name
parameter_values.update(
    {
        "Negative electrode diffusivity [m2.s-1]": parameter_values["Negative particle diffusivity [m2.s-1]"],
    },
    check_already_exists=False,
)
del parameter_values["Negative particle diffusivity [m2.s-1]"]

# Try to run a simulation with the old-style parameter set
model = pybamm.lithium_ion.DFN()
experiment = pybamm.Experiment(
    [
        "Discharge at 1C until 3 V",
    ]
)
sim = pybamm.Simulation(
    model,
    parameter_values=parameter_values,
    experiment=experiment,
)

# KeyError provoked here on call into pybamm.ParameterValues with new key name
sol = sim.solve()
```

### Relevant log output

_No response_

Pull request:
Fixes #4176 electrode diffusivity rename
# Description

Fixes #4176. In addition to this issue, in the `develop` implementation changes to `electrode diffusivity` via `parameter_values.update()` will not be captured. To account for this, the I've taken the depreciated key and appended the new key into parameter_set dictionary in the event that the depreciated parameter is supplied. This also cleans up the `bpx.py` implementation since the logic for changing the key is moved into the parameters class.

Fixes # (issue)

## Type of change

Please add a line in the relevant section of [CHANGELOG.md](https://github.com/pybamm-team/PyBaMM/blob/develop/CHANGELOG.md) to document the change (include PR #) - note reverse order of PR #s. If necessary, also add to the list of breaking changes.

- [ ] New feature (non-breaking change which adds functionality)
- [ ] Optimization (back-end change that speeds up the code)
- [X] Bug fix (non-breaking change which fixes an issue)

# Key checklist:

- [ ] No style issues: `$ pre-commit run` (or `$ nox -s pre-commit`) (see [CONTRIBUTING.md](https://github.com/pybamm-team/PyBaMM/blob/develop/CONTRIBUTING.md#installing-and-using-pre-commit) for how to set this up to run automatically when committing locally, in just two lines of code)
- [ ] All tests pass: `$ python run-tests.py --all` (or `$ nox -s tests`)
- [ ] The documentation builds: `$ python run-tests.py --doctest` (or `$ nox -s doctests`)

You can run integration tests, unit tests, and doctests together at once, using `$ python run-tests.py --quick` (or `$ nox -s quick`).

## Further checks:

- [ ] Code is commented, particularly in hard-to-understand areas
- [ ] Tests added that prove fix is effective or that feature works


Patch:
diff --git a/CHANGELOG.md b/CHANGELOG.md
index ae4616a2d..8dfb4a7a1 100644
--- a/CHANGELOG.md
+++ b/CHANGELOG.md
@@ -24,6 +24,7 @@
 
 ## Bug Fixes
 
+- Fixed bug where passing deprecated `electrode diffusivity` parameter resulted in a breaking change and/or the corresponding diffusivity parameter not updating. Improved the deprecated translation around BPX. ([#4176](https://github.com/pybamm-team/PyBaMM/pull/4176))
 - Fixed a bug where a factor of electrode surface area to volume ratio is missing in the rhs of the LeadingOrderDifferential conductivity model ([#4139](https://github.com/pybamm-team/PyBaMM/pull/4139))
 - Fixes the breaking changes caused by [#3624](https://github.com/pybamm-team/PyBaMM/pull/3624), specifically enables the deprecated parameter `electrode diffusivity` to be used by `ParameterValues.update({name:value})` and `Solver.solve(inputs={name:value})`. Fixes parameter translation from old name to new name, with corrected tests. ([#4072](https://github.com/pybamm-team/PyBaMM/pull/4072)
 - Set the `remove_independent_variables_from_rhs` to `False` by default, and moved the option from `Discretisation.process_model` to `Discretisation.__init__`. This fixes a bug related to the discharge capacity, but may make the simulation slower in some cases. To set the option to `True`, use `Simulation(..., discretisation_kwargs={"remove_independent_variables_from_rhs": True})`. ([#4020](https://github.com/pybamm-team/PyBaMM/pull/4020))
diff --git a/pybamm/parameters/bpx.py b/pybamm/parameters/bpx.py
index 63cb3008d..680477a74 100644
--- a/pybamm/parameters/bpx.py
+++ b/pybamm/parameters/bpx.py
@@ -336,21 +336,16 @@ def _bpx_to_param_dict(bpx: BPX) -> dict:
                 0.0,
             )
             pybamm_dict[
-                phase_domain_pre_name.replace("electrode", "particle")
-                + "diffusivity activation energy [J.mol-1]"
+                phase_domain_pre_name + "diffusivity activation energy [J.mol-1]"
             ] = Ea_D
             D_ref = pybamm_dict[phase_domain_pre_name + "diffusivity [m2.s-1]"]
 
             if callable(D_ref):
-                pybamm_dict[
-                    phase_domain_pre_name.replace("electrode", "particle")
-                    + "diffusivity [m2.s-1]"
-                ] = partial(_diffusivity, D_ref=D_ref, Ea=Ea_D)
+                pybamm_dict[phase_domain_pre_name + "diffusivity [m2.s-1]"] = partial(
+                    _diffusivity, D_ref=D_ref, Ea=Ea_D
+                )
             elif isinstance(D_ref, tuple):
-                pybamm_dict[
-                    phase_domain_pre_name.replace("electrode", "particle")
-                    + "diffusivity [m2.s-1]"
-                ] = partial(
+                pybamm_dict[phase_domain_pre_name + "diffusivity [m2.s-1]"] = partial(
                     _diffusivity,
                     D_ref=partial(
                         _interpolant_func, name=D_ref[0], x=D_ref[1][0], y=D_ref[1][1]
@@ -358,10 +353,9 @@ def _bpx_to_param_dict(bpx: BPX) -> dict:
                     Ea=Ea_D,
                 )
             else:
-                pybamm_dict[
-                    phase_domain_pre_name.replace("electrode", "particle")
-                    + "diffusivity [m2.s-1]"
-                ] = partial(_diffusivity, D_ref=D_ref, Ea=Ea_D, constant=True)
+                pybamm_dict[phase_domain_pre_name + "diffusivity [m2.s-1]"] = partial(
+                    _diffusivity, D_ref=D_ref, Ea=Ea_D, constant=True
+                )
 
     # electrolyte
     Ea_D_e = pybamm_dict.get(
diff --git a/pybamm/parameters/parameter_values.py b/pybamm/parameters/parameter_values.py
index a223ff061..c8011c735 100644
--- a/pybamm/parameters/parameter_values.py
+++ b/pybamm/parameters/parameter_values.py
@@ -392,7 +392,7 @@ class ParameterValues:
 
     @staticmethod
     def check_parameter_values(values):
-        for param in values:
+        for param in list(values.keys()):
             if "propotional term" in param:
                 raise ValueError(
                     f"The parameter '{param}' has been renamed to "
@@ -405,12 +405,13 @@ class ParameterValues:
                     f"parameter '{param}' has been renamed to " "'Thermodynamic factor'"
                 )
             if "electrode diffusivity" in param:
+                new_param = param.replace("electrode", "particle")
                 warn(
-                    f"The parameter '{param}' has been renamed to '{param.replace('electrode', 'particle')}'",
+                    f"The parameter '{param}' has been renamed to '{new_param}'",
                     DeprecationWarning,
                     stacklevel=2,
                 )
-                param = param.replace("electrode", "particle")
+                values[new_param] = values.get(param)
 
         return values
 
diff --git a/pybamm/util.py b/pybamm/util.py
index 69066dcb1..0fe02c835 100644
--- a/pybamm/util.py
+++ b/pybamm/util.py
@@ -57,14 +57,14 @@ class FuzzyDict(dict):
         try:
             return super().__getitem__(key)
         except KeyError as error:
-            if "electrode diffusivity" in key:
+            if "particle diffusivity" in key:
                 warn(
-                    f"The parameter '{key.replace('electrode', 'particle')}' "
+                    f"The parameter '{key.replace('particle', 'electrode')}' "
                     f"has been renamed to '{key}'",
                     DeprecationWarning,
                     stacklevel=2,
                 )
-                return super().__getitem__(key.replace("electrode", "particle"))
+                return super().__getitem__(key.replace("particle", "electrode"))
             if key in ["Negative electrode SOC", "Positive electrode SOC"]:
                 domain = key.split(" ")[0]
                 raise KeyError(


Test patch:
diff --git a/tests/unit/test_parameters/test_bpx.py b/tests/unit/test_parameters/test_bpx.py
index dbaa2b36c..916eb8d16 100644
--- a/tests/unit/test_parameters/test_bpx.py
+++ b/tests/unit/test_parameters/test_bpx.py
@@ -8,6 +8,8 @@ import unittest
 import json
 import pybamm
 import copy
+import numpy as np
+import pytest
 
 
 class TestBPX(TestCase):
@@ -107,28 +109,51 @@ class TestBPX(TestCase):
         }
 
     def test_bpx(self):
-        bpx_obj = copy.copy(self.base)
+        bpx_objs = [
+            {
+                **copy.deepcopy(self.base),
+                "Parameterisation": {
+                    **copy.deepcopy(self.base["Parameterisation"]),
+                    "Negative electrode": {
+                        **copy.deepcopy(
+                            self.base["Parameterisation"]["Negative electrode"]
+                        ),
+                        "Diffusivity [m2.s-1]": "8.3e-13 * exp(-13.4 * x) + 9.6e-15",  # new diffusivity
+                    },
+                },
+            },
+            copy.copy(self.base),
+        ]
 
-        filename = "tmp.json"
-        with tempfile.NamedTemporaryFile(
-            suffix=filename, delete=False, mode="w"
-        ) as tmp:
-            # write to a tempory file so we can
-            # get the source later on using inspect.getsource
-            # (as long as the file still exists)
-            json.dump(bpx_obj, tmp)
-            tmp.flush()
+        model = pybamm.lithium_ion.DFN()
+        experiment = pybamm.Experiment(
+            [
+                "Discharge at C/5 for 1 hour",
+            ]
+        )
 
-            pv = pybamm.ParameterValues.create_from_bpx(tmp.name)
+        filename = "tmp.json"
+        sols = []
+        for obj in bpx_objs:
+            with tempfile.NamedTemporaryFile(
+                suffix=filename, delete=False, mode="w"
+            ) as tmp:
+                # write to a temporary file so we can
+                # get the source later on using inspect.getsource
+                # (as long as the file still exists)
+                json.dump(obj, tmp)
+                tmp.flush()
+
+                pv = pybamm.ParameterValues.create_from_bpx(tmp.name)
+                sim = pybamm.Simulation(
+                    model, parameter_values=pv, experiment=experiment
+                )
+                sols.append(sim.solve())
 
-            model = pybamm.lithium_ion.DFN()
-            experiment = pybamm.Experiment(
-                [
-                    "Discharge at C/5 for 1 hour",
-                ]
+        with pytest.raises(AssertionError):
+            np.testing.assert_allclose(
+                sols[0]["Voltage [V]"].data, sols[1]["Voltage [V]"].data, atol=1e-7
             )
-            sim = pybamm.Simulation(model, parameter_values=pv, experiment=experiment)
-            sim.solve()
 
     def test_constant_functions(self):
         bpx_obj = copy.copy(self.base)
diff --git a/tests/unit/test_util.py b/tests/unit/test_util.py
index 2daad1f6a..603af5056 100644
--- a/tests/unit/test_util.py
+++ b/tests/unit/test_util.py
@@ -36,7 +36,7 @@ class TestUtil(TestCase):
                 "SEI current": 3,
                 "Lithium plating current": 4,
                 "A dimensional variable [m]": 5,
-                "Positive particle diffusivity [m2.s-1]": 6,
+                "Positive electrode diffusivity [m2.s-1]": 6,
             }
         )
         self.assertEqual(d["test"], 1)
@@ -59,7 +59,10 @@ class TestUtil(TestCase):
             d.__getitem__("Open-circuit voltage at 100% SOC [V]")
 
         with self.assertWarns(DeprecationWarning):
-            self.assertEqual(d["Positive electrode diffusivity [m2.s-1]"], 6)
+            self.assertEqual(
+                d["Positive electrode diffusivity [m2.s-1]"],
+                d["Positive particle diffusivity [m2.s-1]"],
+            )
 
     def test_get_parameters_filepath(self):
         tempfile_obj = tempfile.NamedTemporaryFile("w", dir=".")