Rename parameter "Exchange-current density for plating" to "Exchange-current density for lithium metal electrode" #3429
Conversation
| @@ -147,6 +147,10 @@ def test_check_parameter_values(self): | |||
| # since + has other meanings in regex | |||
| with self.assertRaisesRegex(ValueError, "Thermodynamic factor"): | |||
| pybamm.ParameterValues({"1 + dlnf/dlnc": 1}) | |||
| # renamed to "Exchange-current density for lithium-metal electrode [A.m-2]" | |||
There was a problem hiding this comment.
I used assertRaisesRegex here to catch the error but it had problems parsing the minus sign (-) in the units, so I ended up switching to the usual assertRaises instead. Please let me know if this is correct (works fine for me because the error is indeed raised, the method of catching the error is slightly different)
There was a problem hiding this comment.
I usually test a substring that works e.g. "Exchange-current density for plating"
There was a problem hiding this comment.
Okay, I managed to fix that using raw strings in order to keep regex happy.
I do have a separate question about how or if the units should be accounted for: because if the units are to be included in the error checking, Exchange-current density for plating [A.m-2] will raise an error as expected, but Exchange-current density for plating (without the units) will not
Codecov ReportAll modified lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## develop #3429 +/- ##
========================================
Coverage 99.58% 99.58%
========================================
Files 256 256
Lines 19998 20000 +2
========================================
+ Hits 19915 19917 +2
Misses 83 83
☔ View full report in Codecov by Sentry. |
|
Tagging @Saransh-cpp as this should be included in the release |
| @@ -147,6 +147,10 @@ def test_check_parameter_values(self): | |||
| # since + has other meanings in regex | |||
| with self.assertRaisesRegex(ValueError, "Thermodynamic factor"): | |||
| pybamm.ParameterValues({"1 + dlnf/dlnc": 1}) | |||
| # renamed to "Exchange-current density for lithium-metal electrode [A.m-2]" | |||
There was a problem hiding this comment.
I usually test a substring that works e.g. "Exchange-current density for plating"
|
There is a reference for "plating" here however, another reference for "lithium metal electrode" already exists here which one should I keep or modify? |
|
Both, one is for the plating on the lithium meta electrode and the other is for plating in a porous electrode Tagging @DrSOKane to make sure the changes get made correctly |
|
Makes sense, we would have to rename one of them to keep both of them though because we can't have duplicate keys |
agriyakhetarpal
changed the title
|
What do you mean? They should both be kept as-is for that parameter set (I believe the reason for introducing the "Exchange-current density for lithium metal electrode" name was precisely to avoid this conflict) |
|
Sorry instructions weren't clear on how to fix this, I'll open a new PR with the fix |
Description
Closes #3428, adds a check for the use of the "Exchange-current density for plating [A.m-2]" parameter because it is to be renamed to "Exchange-current density for lithium-metal electrode [A.m-2]"
Type of change
Please add a line in the relevant section of CHANGELOG.md to document the change (include PR #) - note reverse order of PR #s. If necessary, also add to the list of breaking changes.
Key checklist:
$ pre-commit run(or$ nox -s pre-commit) (see CONTRIBUTING.md for how to set this up to run automatically when committing locally, in just two lines of code)$ python run-tests.py --all(or$ nox -s tests)$ 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: