Fixes pybamm-team#4176 electrode diffusivity rename (pybamm-team#4179)

* fix: revert logic change for failing 'particle diffusivity' key, updt. check_parameter_values to rename depreciated key * fix: removes bpx remapping requirement, creates 'particle diffusivity' without destroying 'electrode diffusivity', updts tests to verify functionality * Add changelog * Apply suggestions from code review Co-authored-by: Ferran Brosa Planella <[email protected]> Co-authored-by: Agriya Khetarpal <[email protected]> --------- Co-authored-by: Ferran Brosa Planella <[email protected]> Co-authored-by: Agriya Khetarpal <[email protected]>
agriyakhetarpal · Jun 21, 2024 · 23bebd0 · 23bebd0
1 parent 088ca38
commit 23bebd0
Show file tree

Hide file tree

Showing 6 changed files with 64 additions and 40 deletions.
diff --git 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
@@ -336,32 +336,26 @@ def _conductivity(c_e, T, Ea, sigma_ref, constant=False):
                 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]
                     ),
                     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
@@ -392,7 +392,7 @@ def set_initial_ocps(
 
     @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 @@ def check_parameter_values(values):
                     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
@@ -57,14 +57,14 @@ def __getitem__(self, key):
         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(

diff --git a/tests/unit/test_parameters/test_bpx.py b/tests/unit/test_parameters/test_bpx.py
@@ -8,6 +8,8 @@
 import json
 import pybamm
 import copy
+import numpy as np
+import pytest
 
 
 class TestBPX(TestCase):
@@ -107,28 +109,51 @@ def setUp(self):
         }
 
     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
@@ -36,7 +36,7 @@ def test_fuzzy_dict(self):
                 "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 @@ def test_fuzzy_dict(self):
             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=".")