Repository: pybamm-team/PyBaMM

Issue:
[Bug]: `Particle electrode diffusivity` rename breaks ParameterSet.update()
### PyBaMM Version

[Develop]

### Python Version

3.12

### Describe the bug

The parameter rename of `Positive electrode diffusivity` -> `Positive particle diffusivity` currently breaks the `ParameterSet.update()` method.

Linking https://github.com/pybamm-team/PyBaMM/pull/3624

### Steps to Reproduce

```python
import pybamm
parameter_values = pybamm.ParameterValues("Chen2020")
parameter_values.update({"Positive electrode diffusivity [m2.s-1]": 1e-14})
```

### Relevant log output

```shell
{
	"name": "KeyError",
	"message": "\"Cannot update parameter 'Positive electrode diffusivity [m2.s-1]' as it does not have a default value. ('Positive electrode diffusivity [m2.s-1]' not found. Best matches are ['Positive particle diffusivity [m2.s-1]', 'Positive electrode conductivity [S.m-1]', 'Electrolyte diffusivity [m2.s-1]']). If you are sure you want to update this parameter, use param.update({{name: value}}, check_already_exists=False)\"",
	"stack": "---------------------------------------------------------------------------
KeyError                                  Traceback (most recent call last)
File ~/Documents/Git/PyBaMM/pybamm/util.py:59, in FuzzyDict.__getitem__(self, key)
     58 try:
---> 59     return super().__getitem__(key)
     60 except KeyError as error:

KeyError: 'Positive electrode diffusivity [m2.s-1]'

The above exception was the direct cause of the following exception:

KeyError                                  Traceback (most recent call last)
File ~/Documents/Git/PyBaMM/pybamm/parameters/parameter_values.py:247, in ParameterValues.update(self, values, check_conflict, check_already_exists, path)
    246 try:
--> 247     self._dict_items[name]
    248 except KeyError as err:

File ~/Documents/Git/PyBaMM/pybamm/util.py:103, in FuzzyDict.__getitem__(self, key)
    100         raise KeyError(
    101             f\"'{key}' not found. Use the dimensional version '{k}' instead.\"
    102         ) from error
--> 103 raise KeyError(
    104     f\"'{key}' not found. Best matches are {best_matches}\"
    105 ) from error

KeyError: \"'Positive electrode diffusivity [m2.s-1]' not found. Best matches are ['Positive particle diffusivity [m2.s-1]', 'Positive electrode conductivity [S.m-1]', 'Electrolyte diffusivity [m2.s-1]']\"

The above exception was the direct cause of the following exception:

KeyError                                  Traceback (most recent call last)
Cell In[4], line 2
      1 parameter_values = pybamm.ParameterValues(\"Chen2020\")
----> 2 parameter_values.update({\"Positive electrode diffusivity [m2.s-1]\": 1e-14})

File ~/Documents/Git/PyBaMM/pybamm/parameters/parameter_values.py:249, in ParameterValues.update(self, values, check_conflict, check_already_exists, path)
    247         self._dict_items[name]
    248     except KeyError as err:
--> 249         raise KeyError(
    250             f\"Cannot update parameter '{name}' as it does not \"
    251             + f\"have a default value. ({err.args[0]}). If you are \"
    252             + \"sure you want to update this parameter, use \"
    253             + \"param.update({{name: value}}, check_already_exists=False)\"
    254         ) from err
    255 # if no conflicts, update
    256 if isinstance(value, str):

KeyError: \"Cannot update parameter 'Positive electrode diffusivity [m2.s-1]' as it does not have a default value. ('Positive electrode diffusivity [m2.s-1]' not found. Best matches are ['Positive particle diffusivity [m2.s-1]', 'Positive electrode conductivity [S.m-1]', 'Electrolyte diffusivity [m2.s-1]']). If you are sure you want to update this parameter, use param.update({{name: value}}, check_already_exists=False)\""
}
```


Discussion:
codecov[bot]: ## [Codecov](https://app.codecov.io/gh/pybamm-team/PyBaMM/pull/4072?dropdown=coverage&src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=pybamm-team) Report
All modified and coverable lines are covered by tests :white_check_mark:
> Project coverage is 99.59%. Comparing base [(`448450b`)](https://app.codecov.io/gh/pybamm-team/PyBaMM/commit/448450b12d200a056c48e17d5f18d2888421f6f3?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=pybamm-team) to head [(`bc30a67`)](https://app.codecov.io/gh/pybamm-team/PyBaMM/pull/4072?dropdown=coverage&src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=pybamm-team).
> Report is 3 commits behind head on develop.


<details><summary>Additional details and impacted files</summary>


```diff
@@           Coverage Diff            @@
##           develop    #4072   +/-   ##
========================================
  Coverage    99.59%   99.59%           
========================================
  Files          259      259           
  Lines        21353    21357    +4     
========================================
+ Hits         21266    21270    +4     
  Misses          87       87           
```



</details>

[:umbrella: View full report in Codecov by Sentry](https://app.codecov.io/gh/pybamm-team/PyBaMM/pull/4072?dropdown=coverage&src=pr&el=continue&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=pybamm-team).   
:loudspeaker: Have feedback on the report? [Share it here](https://about.codecov.io/codecov-pr-comment-feedback/?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=pybamm-team).

kratman: Out of curiosity, if we did the renaming of the parameter, where is the old name coming from in PyBaMM usage?

In #4071 it looks like you manually passed the old name into the function. Is there another tool in the PyBaMM ecosystem that is still using the old name?
BradyPlanden: Fair question. The previous name would be used in any script/notebook/package built before v24.5, and in those cases, if `ParameterSet.update()` or `Solver.solve(inputs={old_name:value})` is called, the change would be breaking. #3624 both renames the parameter and does the conversion in the [FuzzyDict](https://github.com/pybamm-team/PyBaMM/blob/fadf094d17d0562319e4d5065a2eee1a9848b004/pybamm/util.py#L60), but misses the above methods. This PR fixes that.
kratman: I know we have deprecation warnings for the parameter name changes, I think if anything we should update them to catch it better. Putting it is the base solver seems like the wrong approach. Once we are out of the deprecation window then we would expect to be able to remove the code checking for it.


BradyPlanden: > Putting it is the base solver seems like the wrong approach. Once we are out of the deprecation window then we would expect to be able to remove the code checking for it.

I agree it's not a very clean solution, but given the `inputs` arg is a user defined dictionary and directly passed from user-level I don't believe we can catch it before that point. Unless you see an alternative?

How do we track removal of depreciation warnings at the moment? Perhaps adding depreciation warnings around the additions in this PR and adding them to a log for removal after two release would ensure they get removed when ready.
brosaplanella: We discussed about deprecation warnings in https://github.com/pybamm-team/PyBaMM/issues/2028 a while ago, but not much progress has been made.
valentinsulzer: Can you share the scripts that are broken on `develop` and work with this fix?
kratman: Summary of my comments from the meeting: "Can the check for the old parameter name be done only in the update function and add a warning"
BradyPlanden: > Can you share the scripts that are broken on develop and work with this fix?

Sure @valentinsulzer, [any of these](https://github.com/pybamm-team/PyBaMM/tree/develop/docs/source/examples/notebooks/parameterization) with the old parameter name used in `parameter_values.update()` or the inputs object for `solve()` will break in develop and be fixed by this PR. It's any time `ParameterSet.update({name:value})` or `solver.solve(input={name:value})` uses the previous parameter name, both of which are used throughout the examples.

> "Can the check for the old parameter name be done only in the update function and add a warning"
Yep, I'll sort this and drop a line once it's ready for review.
valentinsulzer: Nice preview of how painful it is to rename a parameter, for you guys leading the parameter refactor effort 
BradyPlanden: There are a few tests that are failing periodically at the moment, but it seems to be across the repo. For coverage, the lines in question are being missed periodically, but not as a function of this PR. This should be good for the final review now.

Patch:
diff --git a/CHANGELOG.md b/CHANGELOG.md
index eb02ebedb..b72151f36 100644
--- a/CHANGELOG.md
+++ b/CHANGELOG.md
@@ -21,6 +21,7 @@
 
 ## Bug Fixes
 
+- 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))
 - Fixed a bug where independent variables were removed from models even if they appeared in events ([#4019](https://github.com/pybamm-team/PyBaMM/pull/4019))
 - Fix bug with upwind and downwind schemes producing the wrong discretised system ([#3979](https://github.com/pybamm-team/PyBaMM/pull/3979))
diff --git a/pybamm/parameters/parameter_values.py b/pybamm/parameters/parameter_values.py
index 402000b5a..830177104 100644
--- a/pybamm/parameters/parameter_values.py
+++ b/pybamm/parameters/parameter_values.py
@@ -5,6 +5,7 @@ import numpy as np
 import pybamm
 import numbers
 from pprint import pformat
+from warnings import warn
 from collections import defaultdict
 
 
@@ -229,7 +230,7 @@ class ParameterValues:
         if not isinstance(values, dict):
             values = values._dict_items
         # check parameter values
-        self.check_parameter_values(values)
+        values = self.check_parameter_values(values)
         # update
         for name, value in values.items():
             # check for conflicts
@@ -389,7 +390,8 @@ class ParameterValues:
         )
         return parameter_values
 
-    def check_parameter_values(self, values):
+    @staticmethod
+    def check_parameter_values(values):
         for param in values:
             if "propotional term" in param:
                 raise ValueError(
@@ -402,6 +404,15 @@ class ParameterValues:
                 raise ValueError(
                     f"parameter '{param}' has been renamed to " "'Thermodynamic factor'"
                 )
+            if "electrode diffusivity" in param:
+                warn(
+                    f"The parameter '{param}' has been renamed to '{param.replace('electrode', 'particle')}'",
+                    DeprecationWarning,
+                    stacklevel=2,
+                )
+                param = param.replace("electrode", "particle")
+
+        return values
 
     def process_model(self, unprocessed_model, inplace=True):
         """Assign parameter values to a model.
diff --git a/pybamm/solvers/base_solver.py b/pybamm/solvers/base_solver.py
index 625de7a3a..798ff9f19 100644
--- a/pybamm/solvers/base_solver.py
+++ b/pybamm/solvers/base_solver.py
@@ -12,6 +12,7 @@ import numpy as np
 
 import pybamm
 from pybamm.expression_tree.binary_operators import _Heaviside
+from pybamm import ParameterValues
 
 
 class BaseSolver:
@@ -1401,7 +1402,10 @@ class BaseSolver:
     @staticmethod
     def _set_up_model_inputs(model, inputs):
         """Set up input parameters"""
-        inputs = inputs or {}
+        if inputs is None:
+            inputs = {}
+        else:
+            inputs = ParameterValues.check_parameter_values(inputs)
 
         # Go through all input parameters that can be found in the model
         # Only keep the ones that are actually used in the model
diff --git a/pybamm/util.py b/pybamm/util.py
index ac7e5f647..e5ec0d812 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 "particle diffusivity" in key:
+            if "electrode diffusivity" in key:
                 warn(
-                    f"The parameter '{key.replace('particle', 'electrode')}' "
+                    f"The parameter '{key.replace('electrode', 'particle')}' "
                     f"has been renamed to '{key}'",
                     DeprecationWarning,
                     stacklevel=2,
                 )
-                return super().__getitem__(key.replace("particle", "electrode"))
+                return super().__getitem__(key.replace("electrode", "particle"))
             if key in ["Negative electrode SOC", "Positive electrode SOC"]:
                 domain = key.split(" ")[0]
                 raise KeyError(


Test patch:
diff --git a/tests/unit/test_util.py b/tests/unit/test_util.py
index 603af5056..2daad1f6a 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 electrode diffusivity [m2.s-1]": 6,
+                "Positive particle diffusivity [m2.s-1]": 6,
             }
         )
         self.assertEqual(d["test"], 1)
@@ -59,10 +59,7 @@ 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]"],
-                d["Positive particle diffusivity [m2.s-1]"],
-            )
+            self.assertEqual(d["Positive electrode diffusivity [m2.s-1]"], 6)
 
     def test_get_parameters_filepath(self):
         tempfile_obj = tempfile.NamedTemporaryFile("w", dir=".")