diff --git a/CHANGELOG.md b/CHANGELOG.md index ae4616a2d4..8dfb4a7a18 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 63cb3008d2..680477a74b 100644 --- a/pybamm/parameters/bpx.py +++ b/pybamm/parameters/bpx.py @@ -336,21 +336,16 @@ 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] @@ -358,10 +353,9 @@ def _conductivity(c_e, T, Ea, sigma_ref, constant=False): 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 a223ff061f..c8011c735c 100644 --- 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 index 69066dcb13..0fe02c835b 100644 --- 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 index dbaa2b36c0..916eb8d161 100644 --- 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 index 2daad1f6a3..603af50560 100644 --- 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=".")