-
-
Notifications
You must be signed in to change notification settings - Fork 529
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.
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
Lithium plating #1287
Lithium plating #1287
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @DrSOKane , this looks almost ready already!
Just a few minor comments. I'd also like to double-check the non-dimensionalization.
Am I right in reading that the "irreversible" and "reversible" plating models are identical except for the form of j_stripping
? If so, we could remove some repeated code by passing options
to the submodel and then using the reversible or irreversible form based on that.
Also, the plating options should be added to BaseModel.options
and there should be a corresponding set_li_plating_models
function
pybamm/models/submodels/interface/li_plating/irreversible_plating.py
Outdated
Show resolved
Hide resolved
if f"{self.domain} electrode sei film overpotential" in variables: | ||
eta_sei = variables[f"{self.domain} electrode sei film overpotential"] | ||
else: | ||
eta_sei = pybamm.Scalar(0) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ideally the sei film overpotential should always be defined, sometimes to zero. Otherwise, if the submodels are called in the wrong order, the sei film overpotential might be non-zero but this would miss it
pybamm/models/submodels/interface/li_plating/irreversible_plating.py
Outdated
Show resolved
Hide resolved
|
||
def __init__(self, param, domain): | ||
super().__init__(param, domain) | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
register citation for the model
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this still needs to be done (unless you don't want to of course)
pybamm/models/submodels/interface/li_plating/reversible_plating.py
Outdated
Show resolved
Hide resolved
""" | ||
|
||
def __init__(self, param, domain): | ||
super().__init__(param, domain) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
register citation for the model
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
still needs to be done
def set_rhs(self, variables): | ||
c_plated_Li = variables[f"{self.domain} electrode Li plating concentration"] | ||
j_stripping = variables[ | ||
f"{self.domain} electrode Li plating " f"interfacial current density" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
second f string not needed
"\n", | ||
"# add plating submodels\n", | ||
"model1.submodels[\"Negative Li plating\"] = pybamm.li_plating.ReversiblePlating(model1.param, \"Negative\")\n", | ||
"model1.submodels[\"Positive Li plating\"] = pybamm.li_plating.NoPlating(model1.param, \"Positive\")\n", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
need to add these to the model options, etc
Codecov Report
@@ Coverage Diff @@
## develop #1287 +/- ##
===========================================
- Coverage 98.15% 98.12% -0.04%
===========================================
Files 272 277 +5
Lines 15600 15750 +150
===========================================
+ Hits 15312 15454 +142
- Misses 288 296 +8
Continue to review full report at Codecov.
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi Simon, that looks very nice! 😃 Just some minor comments.
Apart from the inline comments, you should also add tests for no_plating.py
and base_plating.py
to improve the coverage.
pybamm/models/submodels/interface/li_plating/irreversible_plating.py
Outdated
Show resolved
Hide resolved
"electrolyte": "lipf6_Nyman2008", | ||
"experiment": "1C_discharge_from_full_Chen2020", | ||
"sei": "example", | ||
"citation": "Chen2020", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Apart from Chen2020
you should also add the reference for the plating parameters.
|
||
> Chang-Hui Chen, Ferran Brosa Planella, Kieran O’Regan, Dominika Gastol, W. Dhammika Widanage, and Emma Kendrick. ["Development of Experimental Techniques for Parameterization of Multi-scale Lithium-ion Battery Models."](https://iopscience.iop.org/article/10.1149/1945-7111/ab9050) Journal of the Electrochemical Society 167 (2020): 080534 | ||
|
||
and references therein. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Add the reference you got the plating parameters from
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
General comment (doesn't apply to this PR only): we might need to reconsider how we structure the parameters as the way it is now we repeat ourselves which makes it easier for inconsistencies to appear.
I would say for the moment is fine, but now that we have several different types of models (SEI, plating, mechanics...) it would be good to think if we can improve. Maybe that is a topic for the new Discussion channel?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
can we just append the SEI parameters to the existing set and update the references? or do some of the other parameters change? For the functions that change can they just live in the Chen folder with a new name, and then users manually update that parameter?
Negative electrode OCP entropic change [V.K-1],0,, | ||
,,, | ||
# Li plating parameters,,, | ||
Li metal partial molar volume [m3.mol-1],1.3E-5,, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For the plating parameters, add the reference where you got them from in the third column (like in the Negative electrode charge transfer coefficient, for example).
import unittest | ||
|
||
|
||
class TestReversiblePlating(unittest.TestCase): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think you have a typo here and you want to change all the ReversiblePlating
by IrreversiblePlating
. This would explain why the coverage of irreversible plating is so poor even with these tests.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This still needs to be done (minor fix)
import unittest | ||
|
||
|
||
class TestReversiblePlating(unittest.TestCase): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
To get the coverage of reversible plating to 100% you need to add a couple of tests that cover:
lines 44-45:
else:
eta_sei = pybamm.Scalar(0)
lines 56-63:
if (
"Negative electrode Li plating interfacial current density" in variables
and "Positive electrode Li plating interfacial current density" in variables
and "Li plating interfacial current density" not in variables
):
variables.update(
self._get_standard_whole_cell_interfacial_current_variables(variables)
)
You might need to do the same for the irreversible tests.
Hi all, thanks for your suggestions. I agree the submodel needs more tests, more citations and to merge the reversible and irreversible submodels since they are mostly the same. I've also found that the Li plating model does not converge when SEI is present in large amounts. This is the case even when the code adding eta_sei is commented out. A small amount of SEI does not cause this and produce a reasonable result even after 1000 cycles. |
Is this with constant SEI or varying? |
It works with constant SEI but fails with varying. |
If I understand correctly, when you have both SEI and plating, the model is basically trying to solve an equation of the form |
Two points:
|
Ok makes sense. Would be good to just get something merged (I think coverage is all that is needed) and then we can iterate on it |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good to me, just consider using the full "lithium plating" instead of "Li plating" for legibility, and do you want to register your paper for citations?
@@ -42,6 +42,9 @@ class BaseBatteryModel(pybamm.BaseModel): | |||
* "interfacial surface area" : str, optional | |||
Sets the model for the interfacial surface area. Can be "constant" | |||
(default) or "varying". Not currently implemented in any of the models. | |||
* "Li plating" : str, optional |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Very minor: I would prefer "lithium plating" to "Li plating"
@@ -85,6 +85,29 @@ def set_sei_submodel(self): | |||
# positive electrode | |||
self.submodels["positive sei"] = pybamm.sei.NoSEI(self.param, "Positive") | |||
|
|||
def set_li_plating_submodel(self): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
in general I don't think anything is lost by calling all these things lithium_plating
, in fact makes it easier to read
|
||
def __init__(self, param, domain): | ||
super().__init__(param, domain) | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this still needs to be done (unless you don't want to of course)
""" | ||
|
||
def __init__(self, param, domain): | ||
super().__init__(param, domain) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
still needs to be done
|
||
# negative electrode | ||
if self.options["Li plating"] == "none": | ||
self.submodels["negative Li plating"] = pybamm.sei.NoPlating( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this should say li_plating.NoPlating
. I am surprised the tests didn't pick this up ...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree with @tinosulzer comments. Once this and the additional comment on test_irreversible_plating.py
are fixed we can merge. Thanks @DrSOKane!
import unittest | ||
|
||
|
||
class TestReversiblePlating(unittest.TestCase): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This still needs to be done (minor fix)
Also, note that there are some conflicts with the main branch (in |
I'm in the process of renaming everything to lithium_plating or "lithium plating". While I'm doing that, do you want me to add the stripping current to inverse_butler_volmer.py? |
What does this refer to again? In the |
I am referring to the CurrentForInverseButlerVolmer class, yes. |
Yes that would be good to update too |
Added plating/stripping current density to CurrentForInverseButlerVolmer and fixed remaining conflicts. I've also added my paper to CITATIONS.txt but not sure how to use the BibTeX entry now that it's there! |
You need to add |
Hi all, I'm trying to test if examples/lithium-plating.ipynb still runs and I'm getting the pybtex error: ModuleNotFoundError Traceback (most recent call last) ~/PyBaMM/pybamm/init.py in ~/PyBaMM/pybamm/citations.py in ModuleNotFoundError: No module named 'pybtex' |
You need to install pybtex, just do |
I think the example tests will still fail because |
examples/scripts/custom_model.py
Outdated
@@ -59,6 +59,12 @@ | |||
] = pybamm.electrolyte_conductivity.LeadingOrder(model.param) | |||
model.submodels["negative sei"] = pybamm.sei.NoSEI(model.param, "Negative") | |||
model.submodels["positive sei"] = pybamm.sei.NoSEI(model.param, "Positive") | |||
model.submodels[ | |||
"negative lithium plating" | |||
] = pybamm.li_plating.NoPlating(model.param, "Negative") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This should be pybamm.lithium_plating
(same in the line below). I think it is what is breaking the example tests.
@all-contributors add @DrSOKane for code, examples, documentation and tests |
I've put up a pull request to add @DrSOKane! 🎉 |
Great work @DrSOKane, thank you very much! |
Description
Fixes # 836
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:
$ flake8
$ python run-tests.py --unit
$ cd docs
and then$ make clean; make html
You can run all three at once, using
$ python run-tests.py --quick
.Further checks: