Lithium plating by DrSOKane · Pull Request #1287 · pybamm-team/PyBaMM

DrSOKane · 2020-12-09T22:13:23Z

Description

The reversible Li plating model of O'Kane et al. (2020) is included as a new submodel
An irreversible plating model with similar physics is also included
New parameters that are used by these submodels are defined
A new variant of the existing Chen 2020 parameter set that includes these new parameters has been added
The new parameter set also has temperature-dependent diffusivity in the anode
The submodel accounts for SEI layer resistance
The submodel does NOT account for temperature variation at this time, but this should be easy to fix

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.

New feature (non-breaking change which adds functionality)
Optimization (back-end change that speeds up the code)
Bug fix (non-breaking change which fixes an issue)

Key checklist:

No style issues: $ flake8
All tests pass: $ python run-tests.py --unit
The documentation builds: $ cd docs and then $ make clean; make html

You can run all three at once, using $ python run-tests.py --quick.

Further checks:

Code is commented, particularly in hard-to-understand areas
Tests added that prove fix is effective or that feature works

…develop

valentinsulzer

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

valentinsulzer · 2020-12-09T22:32:56Z

+        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)


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

valentinsulzer · 2020-12-09T22:34:18Z

+
+    def __init__(self, param, domain):
+        super().__init__(param, domain)
+


register citation for the model

this still needs to be done (unless you don't want to of course)

valentinsulzer · 2020-12-09T22:34:42Z

+    """
+
+    def __init__(self, param, domain):
+        super().__init__(param, domain)


register citation for the model

still needs to be done

valentinsulzer · 2020-12-09T22:36:40Z

+    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"


second f string not needed

valentinsulzer · 2020-12-09T22:43:31Z

+    "\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",


need to add these to the model options, etc

codecov · 2020-12-09T23:24:25Z

Codecov Report

Merging #1287 (c3c3d69) into develop (ee3d67c) will decrease coverage by 0.03%.
The diff coverage is 94.73%.

@@             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

Impacted Files	Coverage Δ
...m/models/full_battery_models/base_battery_model.py	`99.04% <50.00%> (-0.64%)`	⬇️
...ttery_models/lithium_ion/base_lithium_ion_model.py	`92.72% <50.00%> (-7.28%)`	⬇️
.../interface/lithium_plating/irreversible_plating.py	`96.87% <96.87%> (ø)`
...ls/interface/lithium_plating/reversible_plating.py	`96.96% <96.96%> (ø)`
...l_battery_models/lead_acid/base_lead_acid_model.py	`100.00% <100.00%> (ø)`
...ybamm/models/full_battery_models/lead_acid/full.py	`100.00% <100.00%> (ø)`
...dels/full_battery_models/lead_acid/higher_order.py	`98.11% <100.00%> (+0.01%)`	⬆️
...ybamm/models/full_battery_models/lead_acid/loqs.py	`100.00% <100.00%> (ø)`
...bamm/models/full_battery_models/lithium_ion/dfn.py	`100.00% <100.00%> (ø)`
...bamm/models/full_battery_models/lithium_ion/spm.py	`100.00% <100.00%> (ø)`
... and 13 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update ee3d67c...145d9a0. Read the comment docs.

brosaplanella

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.

brosaplanella · 2020-12-10T08:15:36Z

+    "electrolyte": "lipf6_Nyman2008",
+    "experiment": "1C_discharge_from_full_Chen2020",
+    "sei": "example",
+    "citation": "Chen2020",


Apart from Chen2020 you should also add the reference for the plating parameters.

brosaplanella · 2020-12-10T08:16:03Z

+
+> 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.


Add the reference you got the plating parameters from

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?

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?

brosaplanella · 2020-12-10T08:16:51Z

+Negative electrode OCP entropic change [V.K-1],0,,
+,,,
+# Li plating parameters,,,
+Li metal partial molar volume [m3.mol-1],1.3E-5,,


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).

brosaplanella · 2020-12-10T08:22:18Z

+import unittest
+
+
+class TestReversiblePlating(unittest.TestCase):


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.

This still needs to be done (minor fix)

brosaplanella · 2020-12-10T08:26:34Z

+import unittest
+
+
+class TestReversiblePlating(unittest.TestCase):


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.

DrSOKane · 2020-12-10T16:44:20Z

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.

valentinsulzer · 2020-12-10T17:12:34Z

Is this with constant SEI or varying?

DrSOKane · 2020-12-10T18:51:41Z

It works with constant SEI but fails with varying.

valentinsulzer · 2020-12-10T19:07:28Z

If I understand correctly, when you have both SEI and plating, the model is basically trying to solve an equation of the form
j(phi_s - phi_e) + j_sei(phi_s - phi_e) + j_pl(phi_s - phi_e) = I
for phi_s - phi_e, where each j is an exponential or sinh. I can see this being numerically challenging. I suggest we deal with it separately from this PR.

DrSOKane · 2021-01-18T12:32:27Z

Two points:

It is true that the two models in this release are identical save for the expression for j_stripping. However, I also have an unpublished model that differs more substantially. My plan is to keep the two separate submodels but add an option to allow the user to easily pick one.
I do not actually have a meaningful reference for the plating constant. It's true that my paper does provide a value for it, but that's not only for a different cell but for a more physically accurate nonlinear diffusion function, which I've been unable to implement in PyBaMM thus far.

valentinsulzer · 2021-01-18T13:22:22Z

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

valentinsulzer

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?

valentinsulzer · 2021-01-20T19:24:19Z

            * "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


Very minor: I would prefer "lithium plating" to "Li plating"

valentinsulzer · 2021-01-20T19:25:42Z

        # positive electrode
        self.submodels["positive sei"] = pybamm.sei.NoSEI(self.param, "Positive")

+    def set_li_plating_submodel(self):


in general I don't think anything is lost by calling all these things lithium_plating, in fact makes it easier to read

valentinsulzer · 2021-01-20T19:26:55Z

+
+    def __init__(self, param, domain):
+        super().__init__(param, domain)
+


this still needs to be done (unless you don't want to of course)

valentinsulzer · 2021-01-20T19:27:47Z

+    """
+
+    def __init__(self, param, domain):
+        super().__init__(param, domain)


still needs to be done

valentinsulzer · 2021-01-20T19:44:16Z

+
+        # negative electrode
+        if self.options["Li plating"] == "none":
+            self.submodels["negative Li plating"] = pybamm.sei.NoPlating(


this should say li_plating.NoPlating. I am surprised the tests didn't pick this up ...

brosaplanella

I agree with @tinosulzer comments. Once this and the additional comment on test_irreversible_plating.py are fixed we can merge. Thanks @DrSOKane!

brosaplanella · 2021-01-21T09:12:43Z

+import unittest
+
+
+class TestReversiblePlating(unittest.TestCase):


This still needs to be done (minor fix)

brosaplanella · 2021-01-21T09:15:31Z

Also, note that there are some conflicts with the main branch (in base_battery_model.py) that also need to be addressed before we can merge it.

DrSOKane · 2021-01-22T13:54:12Z

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?

valentinsulzer · 2021-01-22T16:37:28Z

do you want me to add the stripping current to inverse_butler_volmer.py?

What does this refer to again? In the CurrentForInverseButlerVolmer class?

DrSOKane · 2021-01-22T19:49:15Z

I am referring to the CurrentForInverseButlerVolmer class, yes.

valentinsulzer · 2021-01-27T17:05:14Z

Yes that would be good to update too

DrSOKane · 2021-01-28T19:53:19Z

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!

valentinsulzer · 2021-01-28T20:40:45Z

You need to add pybamm.citations.register("your_paper_bibtex_identifier") to the __init__ of any python class that uses your paper, i.e. the two plating classes. Not sure why the tests didn't start, it might be because there are still some conflicts

…develop

DrSOKane · 2021-02-02T17:55:27Z

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)
in
1 get_ipython().run_line_magic('pip', 'install pybamm -q # install PyBaMM if it is not installed')
----> 2 import pybamm
3 import os
4 import numpy as np
5 import matplotlib.pyplot as plt

~/PyBaMM/pybamm/init.py in
70 from .logger import logger, set_logging_level
71 from .settings import settings
---> 72 from .citations import Citations, citations, print_citations
73
74 #

~/PyBaMM/pybamm/citations.py in
6 import pybamm
7 import os
----> 8 import pybtex
9
10

ModuleNotFoundError: No module named 'pybtex'

valentinsulzer · 2021-02-02T17:57:45Z

You need to install pybtex, just do pip install -e . and it will do it for you

…develop

brosaplanella · 2021-02-03T15:22:38Z

I think the example tests will still fail because lithium_plating.ipynb does not include pybamm.print_citations() at the end (this is a new test we added recently). You can copy the last block from another notebook (for example Tutorial 2 - Compare models.ipynb, don't copy the block in Tutorial 1 as it is slightly different).

brosaplanella · 2021-02-03T17:17:44Z

 model.submodels["positive sei"] = pybamm.sei.NoSEI(model.param, "Positive")
+model.submodels[
+    "negative lithium plating"
+] = pybamm.li_plating.NoPlating(model.param, "Negative")


This should be pybamm.lithium_plating (same in the line below). I think it is what is breaking the example tests.

brosaplanella · 2021-02-03T19:48:21Z

@all-contributors add @DrSOKane for code, examples, documentation and tests

allcontributors · 2021-02-03T19:48:30Z

@brosaplanella

I've put up a pull request to add @DrSOKane! 🎉

brosaplanella · 2021-02-03T19:50:24Z

Great work @DrSOKane, thank you very much!

Simon O'Kane added 8 commits July 22, 2020 10:47

Added updated diffusivity fits into new chemistry Ecker2015_v2

fbedf06

Merge branch 'develop' of https://github.com/pybamm-team/PyBaMM into …

49424c2

…develop

Merge branch 'develop' of https://github.com/pybamm-team/PyBaMM into …

cd36c4c

…develop

Merge branch 'develop' of https://github.com/pybamm-team/PyBaMM into …

30a01b2

…develop

Added li_plating directory

ba31e35

Added remaining files for Li plating model

adb5ff6

Deleted unused parameter set (it might come back later

f155d68

Deleted redundant parameter set properly this time

a67074f

valentinsulzer reviewed Dec 9, 2020

View reviewed changes

brosaplanella requested changes Dec 10, 2020

View reviewed changes

Simon O'Kane added 2 commits January 18, 2021 17:28

Added Li plating to model options and tests for Li plating models

43d22cf

Style changes

e904a8e

valentinsulzer approved these changes Jan 20, 2021

View reviewed changes

valentinsulzer reviewed Jan 20, 2021

View reviewed changes

brosaplanella approved these changes Jan 21, 2021

View reviewed changes

Simon O'Kane added 2 commits January 22, 2021 14:53

Replace Li with lithium, except in variable names

d761d29

Fixed merge conflict in base_battery_model.py

054461a

Fixed more merge conflicts in base_battery_model.py

c975890

Added plating/stripping current density to CurrentForInverseButlerVolmer

d950ac9

Simon O'Kane added 2 commits February 2, 2021 17:49

Merge branch 'develop' of https://github.com/pybamm-team/PyBaMM into …

c3739e3

…develop

Fixed merge conflict in CITATIONS.txt

be023b9

Simon O'Kane added 6 commits February 2, 2021 19:17

Updates: pybtex, anode -> negative electrode, sub_solutions -> cycles

84f9a91

Added NoPlating to lead-acid models

fa05c17

Merge branch 'develop' of https://github.com/pybamm-team/PyBaMM into …

f6be0a4

…develop

Merge branch 'develop' into LithiumPlating

7a5df6e

All tests now pass

d4c076c

Added lithium plating to relevent examples

a18488b

Updated lithium-plating.ipynb with new citations feature

26a5bd2

brosaplanella reviewed Feb 3, 2021

View reviewed changes

Fixed custom_model.py

145d9a0

allcontributors Bot mentioned this pull request Feb 3, 2021

docs: add DrSOKane as a contributor #1357

Merged

brosaplanella merged commit 7fb6040 into pybamm-team:develop Feb 3, 2021

valentinsulzer mentioned this pull request Feb 3, 2021

Add lithium plating model #836

Closed


		def __init__(self, param, domain):
		super().__init__(param, domain)


		> 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.

		import unittest


		class TestReversiblePlating(unittest.TestCase):

Uh oh!

Uh oh!

Conversation

DrSOKane commented Dec 9, 2020

Description

Type of change

Key checklist:

Further checks:

Uh oh!

valentinsulzer left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

codecov Bot commented Dec 9, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

brosaplanella left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

DrSOKane commented Dec 10, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

valentinsulzer commented Dec 10, 2020

Uh oh!

DrSOKane commented Dec 10, 2020

Uh oh!

valentinsulzer commented Dec 10, 2020

Uh oh!

DrSOKane commented Jan 18, 2021

Uh oh!

valentinsulzer commented Jan 18, 2021

Uh oh!

valentinsulzer left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

valentinsulzer Jan 20, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

codecov Bot commented Dec 9, 2020 •

edited

Loading

DrSOKane commented Dec 10, 2020 •

edited

Loading

valentinsulzer Jan 20, 2021 •

edited

Loading

brosaplanella commented Jan 21, 2021 •

edited

Loading

brosaplanella commented Feb 3, 2021 •

edited

Loading