Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Rename parameter "Exchange-current density for plating" to "Exchange-current density for lithium metal electrode" #3429

Closed
wants to merge 7 commits into from
Closed

Rename parameter "Exchange-current density for plating" to "Exchange-current density for lithium metal electrode" #3429

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
8c7157b
#3428 raise error on exchange-current density for plating parameter
agriyakhetarpal Oct 9, 2023
ae7dc73
#3428 check if error is raised + coverage
agriyakhetarpal Oct 9, 2023
92e2a4f
#3428 rename all instances of "plating" to "lithium-metal electrode"
agriyakhetarpal Oct 9, 2023
39e52a0
#3428 add a CHANGELOG entry
agriyakhetarpal Oct 9, 2023
5d917fb
#3428 fix test, add units to error check
agriyakhetarpal Oct 9, 2023
5607acb
#3428 oops, un-swap parameters to raise error with
agriyakhetarpal Oct 10, 2023
c1ac296
#3428 use assertRaises instead of assertRaisesRegex
agriyakhetarpal Oct 10, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions CHANGELOG.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -44,6 +44,7 @@

## Breaking changes

- The parameter "Exchange-current density for plating \[A.m-2]" has been renamed to "Exchange-current density for lithium-metal electrode \[A.m-2]" ([#3429](https://github.com/pybamm-team/PyBaMM/pull/3429))
- Dropped support for i686 (32-bit) architectures on GNU/Linux distributions ([#3412](https://github.com/pybamm-team/PyBaMM/pull/3412))
- The class `pybamm.thermal.OneDimensionalX` has been moved to `pybamm.thermal.pouch_cell.OneDimensionalX` to reflect the fact that the model formulation implicitly assumes a pouch cell geometry ([#3257](https://github.com/pybamm-team/PyBaMM/pull/3257))
- The "lumped" thermal option now always used the parameters "Cell cooling surface area [m2]", "Cell volume [m3]" and "Total heat transfer coefficient [W.m-2.K-1]" to compute the cell cooling regardless of the chosen "cell geometry" option. The user must now specify the correct values for these parameters instead of them being calculated based on e.g. a pouch cell. An `OptionWarning` is raised to let users know to update their parameters ([#3257](https://github.com/pybamm-team/PyBaMM/pull/3257))
Expand Down
2 changes: 1 addition & 1 deletion pybamm/input/parameters/lithium_ion/Mohtat2020.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -342,7 +342,7 @@ def get_parameter_values():
"chemistry": "lithium_ion",
# lithium plating
"Lithium metal partial molar volume [m3.mol-1]": 1.3e-05,
"Exchange-current density for plating [A.m-2]": 0.001,
"Exchange-current density for lithium-metal electrode [A.m-2]": 0.001,
"Initial plated lithium concentration [mol.m-3]": 0.0,
"Typical plated lithium concentration [mol.m-3]": 1000.0,
"Lithium plating transfer coefficient": 0.7,
Expand Down
2 changes: 1 addition & 1 deletion pybamm/input/parameters/lithium_ion/OKane2022.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -515,7 +515,7 @@ def get_parameter_values():
# lithium plating
"Lithium metal partial molar volume [m3.mol-1]": 1.3e-05,
"Lithium plating kinetic rate constant [m.s-1]": 1e-09,
"Exchange-current density for plating [A.m-2]"
"Exchange-current density for lithium-metal electrode [A.m-2]"
"": plating_exchange_current_density_OKane2020,
"Exchange-current density for stripping [A.m-2]"
"": stripping_exchange_current_density_OKane2020,
Expand Down
2 changes: 1 addition & 1 deletion pybamm/input/parameters/lithium_ion/OKane2022_graphite_SiOx_halfcell.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -399,7 +399,7 @@ def get_parameter_values():
# lithium plating
"Lithium metal partial molar volume [m3.mol-1]": 1.3e-05,
"Lithium plating kinetic rate constant [m.s-1]": 1e-09,
"Exchange-current density for plating [A.m-2]"
"Exchange-current density for lithium-metal electrode [A.m-2]"
"": plating_exchange_current_density_OKane2020,
"Exchange-current density for stripping [A.m-2]"
"": stripping_exchange_current_density_OKane2020,
Expand Down
4 changes: 2 additions & 2 deletions pybamm/parameters/lithium_ion_parameters.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -199,14 +199,14 @@ def j0_stripping(self, c_e, c_Li, T):
)

def j0_plating(self, c_e, c_Li, T):
"""Dimensional exchange-current density for plating [A.m-2]"""
"""Dimensional exchange-current density for lithium-metal electrode [A.m-2]"""
inputs = {
"Electrolyte concentration [mol.m-3]": c_e,
"Plated lithium concentration [mol.m-3]": c_Li,
"Temperature [K]": T,
}
return pybamm.FunctionParameter(
"Exchange-current density for plating [A.m-2]", inputs
"Exchange-current density for lithium-metal electrode [A.m-2]", inputs
)

def dead_lithium_decay_rate(self, L_sei):
Expand Down
5 changes: 5 additions & 0 deletions pybamm/parameters/parameter_values.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -324,6 +324,11 @@ def check_parameter_values(self, values):
raise ValueError(
f"parameter '{param}' has been renamed to " "'Thermodynamic factor'"
)
if "Exchange-current density for plating [A.m-2]" in param:
raise ValueError(
f"parameter '{param}' has been renamed to "
"'Exchange-current density for lithium-metal electrode [A.m-2]'"
)

def process_model(self, unprocessed_model, inplace=True):
"""Assign parameter values to a model.
Expand Down
4 changes: 3 additions & 1 deletion tests/unit/test_parameters/test_parameter_sets/test_OKane2022.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,7 +14,9 @@ def test_functions(self):

fun_test = {
# Lithium plating
"Exchange-current density for plating [A.m-2]": ([1e3, 1e4, T], 9.6485e-2),
"Exchange-current density for lithium-metal electrode [A.m-2]": (
[1e3, 1e4, T], 9.6485e-2
),
"Exchange-current density for stripping [A.m-2]": (
[1e3, 1e4, T],
9.6485e-1,
Expand Down
4 changes: 3 additions & 1 deletion tests/unit/test_parameters/test_parameter_sets/test_OKane2022_negative_halfcell.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,7 +14,9 @@ def test_functions(self):

fun_test = {
# Lithium plating
"Exchange-current density for plating [A.m-2]": ([1e3, 1e4, T], 9.6485e-2),
"Exchange-current density for lithium-metal electrode [A.m-2]": (
[1e3, 1e4, T], 9.6485e-2
),
"Exchange-current density for stripping [A.m-2]": (
[1e3, 1e4, T],
9.6485e-1,
Expand Down
4 changes: 4 additions & 0 deletions tests/unit/test_parameters/test_parameter_values.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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]"
Copy link
Member Author

@agriyakhetarpal agriyakhetarpal Oct 10, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I usually test a substring that works e.g. "Exchange-current density for plating"

Copy link
Member Author

@agriyakhetarpal agriyakhetarpal Oct 10, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

with self.assertRaises(ValueError):
pybamm.ParameterValues(
{"Exchange-current density for plating [A.m-2]": 0.001})

def test_process_symbol(self):
parameter_values = pybamm.ParameterValues({"a": 4, "b": 2, "c": 3})
Expand Down
Loading