Issue 836 add lithium plating #1009

felipe-salinas · 2020-05-21T06:49:01Z

Description

Adds SEI model based on Yang et. al. (see #836) and reduction in negative electrode porosity.

Fixes # (issue)

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

Adds new SEI reaction "ec reaction limited"

valentinsulzer

Thanks @felipe-salinas , impressed with how quickly you have got to grips with the submodel structure. Some additional things to do are:

Add the EC SEI model to the sei.rst docs
Add the EC SEI model to the BaseBatteryModel docstring
Add unit and integration tests for using the EC SEI model with the SPM, SPMe, DFN. You can just copy the test for the other SEI models and change the option (see test_spm.py, etc)

It looks like you also undid some changes in the merge, please check this. The automatic tests didn't start because we recently switched from travis to github actions but this should be fixed by the time you push again

valentinsulzer · 2020-05-21T13:21:00Z

examples/scripts/cycling_ageing_yang.py

+
+options = {"sei": "ec reaction limited",
+           "sei film resistance": "distributed",
+           "porosity": "variable porosity"}


Note that the "sei film resistance" is set automatically by the following lines in BaseBatteryModel:

# Change the default for SEI film resistance based on which sei option is # provided extra_options = extra_options or {} sei_option = extra_options.get("sei", None) # return None if option not given if sei_option is None: default_options["sei film resistance"] = None else: default_options["sei film resistance"] = "distributed"

Something similar could be done for porosity

Thanks for the tip: I will erase the redundant setting for sei film resistance in this example. In the case of porosity, It would be a nice feature, but the variable porosity is not a default option for SEI models (older publications like Ramadass et. al. 2003 don't include it), so I think that is better to leave it as a "visible" option.

pybamm/input/parameters/lithium-ion/seis/example/parameters.csv

valentinsulzer · 2020-05-21T13:22:06Z

pybamm/models/full_battery_models/base_battery_model.py

-        ]
-        eta_sei_av = eta_sei_n_av + eta_sei_p_av
-        eta_sei_av_dim = eta_sei_n_av_dim + eta_sei_p_av_dim
-


lines deleted accidentally?

Sorry, I grabbed an older version of develop :/

valentinsulzer · 2020-05-21T13:23:56Z

pybamm/models/full_battery_models/base_battery_model.py

@@ -684,8 +670,6 @@ def set_voltage_variables(self):
                "Measured open circuit voltage [V]": ocv_dim,
                "X-averaged reaction overpotential": eta_r_av,
                "X-averaged reaction overpotential [V]": eta_r_av_dim,
-                "X-averaged sei film overpotential": eta_sei_av,
-                "X-averaged sei film overpotential [V]": eta_sei_av_dim,


lines deleted accidentally?

I think it is correct now.

valentinsulzer · 2020-05-21T13:25:41Z

pybamm/models/full_battery_models/lithium_ion/dfn.py

+        if self.options["porosity"] is None:
+            self.submodels["porosity"] = pybamm.porosity.Constant(self.param)
+        elif self.options["porosity"] == "variable porosity":
+            self.submodels["porosity"] = pybamm.porosity.Full(self.param)


equivalent changes need to be made in SPM and SPMe models

valentinsulzer · 2020-05-21T13:27:13Z

pybamm/models/submodels/interface/sei/ec_reaction_limited.py

+
+        L_scale = pybamm.sei_parameters.L_sei_0_dim
+        # in this model the scale is identical to the intercalation current
+        j_scale = pybamm.sei_parameters.j_scale


j_scale is different in the negaive and positive electrodes so you need to be a bit careful here (though at the moment an error will be raised if the positive electrode sei model isn't NoSEI, so it might be ok)

Modified in the file to explicitly recover the negative scale.

valentinsulzer · 2020-05-21T13:30:31Z

pybamm/models/submodels/interface/sei/ec_reaction_limited.py

+
+        self.algebraic = {j_sei: j_sei + C_sei_ec * c_ec * pybamm.exp(- 0.5 * (
+            phi_s_n - phi_e_n - j * L_sei * R_sei)),
+            c_ec: c_ec - pybamm.Scalar(1) - j_sei * L_sei * C_ec}


it looks like j_sei and c_ec can be defined explicitly in terms of other variables in get_coupled_variables, instead of being made Variable objects themselves. While this will give the same answer, it's best to try to reduce the number of equations, especially algebraic

c_ec and j_sei are present in both equations, and j_sei depends on itself because it is included in j (the total current). I think that makes it necessary to implement j_sei with an algebraic equation. c_ec is now defined by the other variables.

valentinsulzer · 2020-05-21T13:32:15Z

pybamm/models/submodels/porosity/full_reaction_driven_porosity.py

-        deps_p_dt = -self.param.beta_surf_p * j_p
+        # for the moment is not needed but could be used for positive SEI
+        deps_p_dt = pybamm.FullBroadcast(0, "positive electrode",
+                                         "current collector")


The beta_surf_n * j_n and beta_surf_p * j_p terms should be kept as they are used in the lead-acid models. You can do this by setting beta_surf_n and beta_surf_p to zero in standard_parameters_lithium_ion

I created another class because they have different purposes.

valentinsulzer · 2020-05-21T13:33:01Z

pybamm/parameters/sei_parameters.py

-)
+C_sei_reaction = (
+    F * L_sei_0_dim / (m_sei_dimensional * tau_discharge * V_bar_inner_dimensional)
+) * pybamm.exp(-(F * U_n_ref / (2 * R * T_ref)))


please fix the changes overwritten in this file

valentinsulzer · 2020-05-21T13:34:39Z

examples/scripts/cycling_ageing_yang.py

+        "Sum of negative electrode interfacial current densities",
+        #"Negative electrode EC surface concentration",
+    ]
+)


nice example!

valentinsulzer

Thanks @felipe-salinas , getting there, just some more comments and need to add the docs and tests

valentinsulzer · 2020-05-27T13:53:54Z

pybamm/models/full_battery_models/lithium_ion/spm.py

+        if self.options["porosity"] is None:
+            self.submodels["porosity"] = pybamm.porosity.Constant(self.param)
+        elif self.options["porosity"] == "variable porosity":
+            self.submodels["porosity"] = pybamm.porosity.SeiFull(self.param)


For the SPM, given that j_sei is uniform in x, using the "SeiFull" porosity submodel creates unnecessarily many states for the porosity (i.e. when discretised, there will be n ODEs where n is the number of grid points, instead of just one ODE for each domain). You could use the "leading order reaction driven porosity" model instead

valentinsulzer · 2020-05-27T13:54:05Z

pybamm/models/full_battery_models/lithium_ion/spme.py

+        if self.options["porosity"] is None:
+            self.submodels["porosity"] = pybamm.porosity.Constant(self.param)
+        elif self.options["porosity"] == "variable porosity":
+            self.submodels["porosity"] = pybamm.porosity.SeiFull(self.param)


Same comment as for the SPM

valentinsulzer · 2020-05-27T13:59:28Z

pybamm/models/submodels/porosity/sei_reaction_driven_porosity.py

+            )
+        )
+
+        return variables


Still not convinced this warrants a whole separate submodel. All you would need to do to reuse the full submodel is to add these lines to standard_parameters_lithium_ion.py:

beta_surf_n = pybamm.Scalar(0) beta_surf_p = pybamm.Scalar(0)

By the way, just so you are aware, deps_dt comes into the dce_dt equation (see e.g. full_diffusion.py) so a changing porosity will affect the electrolyte concentration (make the electrolyte more concentrated) - again, this comes from the lead-acid models

Merge develop

Merge

valentinsulzer · 2020-06-23T16:20:54Z

Now #1073

felipe-salinas added 9 commits May 21, 2020 08:18

Add files via upload

92c2f45

Add files via upload

d3e32e8

Adds new SEI reaction "ec reaction limited"

Adds default porosity model

ad23518

Include change in porosity due to SEI current

9c9fc62

SEI model based on Yang et. al. 2017

5230337

Parameters for SEI reaction Yang et. al. 2017

b6a1cb8

Experiment of the related paper

b9d0187

Update CHANGELOG.md

fa26ec9

Needed for initial conditions of porosity

587a177

valentinsulzer mentioned this pull request May 21, 2020

CI tests are not run on PRs from forks #1012

Closed

valentinsulzer requested changes May 21, 2020

View reviewed changes

felipe-salinas mentioned this pull request May 25, 2020

Add lithium plating model #836

Closed

felipe-salinas added 6 commits May 25, 2020 13:47

Correct file with existing develop

31df1a4

Corrects porosity used and added to other models

37aa4a2

Creates new porosity class

ecf53a7

Corrects unnecessary changes to file from develop

62d7004

Negative current scale

41b6606

c_ec defined in get_coupled_variables

50e6ad5

valentinsulzer requested changes May 27, 2020

View reviewed changes

felipe-salinas added 11 commits June 2, 2020 13:07

Add sei current and associated beta coefficient

e6ee84b

Original file

90162c5

adds Full porosity

2b8d03c

Adds leading order porosity

27342fd

Adds leading order porosity

bd65b49

Beta coefficients and initial condition porosity

ddc9e40

Adds X-averaged j_sei

b42b5a8

Adds ec reaction limited

015931a

Add test for ec reaction limited

de368e2

Adds test for ex reaction limited

f31be98

Adds reference performance tests

a16fdaf

felipe-salinas added 26 commits June 2, 2020 13:22

Replaced by full order

384220a

adds Ramadass2004

966f908

Create dummy

022b613

Graphite of referenced paper

d58c8c4

Delete dummy

0a058b9

Create dummy

be3838c

Cathode of referenced paper

4029d0a

Create dummy

13e781e

Cell geometry

8a68dc9

Delete dummy

3afdfb5

Delete dummy

cd0f9ba

Create dummy

f415bcf

Electrolyte from reference

57cea0e

Delete dummy

91a598d

Create dummy

f8024f0

Add files via upload

0433800

Delete dummy

248e776

Create dummy

67b04b0

A separate type to allow changes in example

a43b93d

Delete dummy

5740132

Merge pull request #1 from pybamm-team/develop

49a490c

Merge develop

Merge branch 'issue-836-add-lithium-plating' into Update

ce006e7

Merge pull request #2 from felipe-salinas/Update

2359e1f

Merge

Update sei_parameters.py

c70e1fc

Update README.md

2f5295d

Update README.md

5f7f7cf

valentinsulzer mentioned this pull request Jun 23, 2020

Issue 836 add lithium plating #1073

Merged

8 tasks

valentinsulzer closed this Jun 23, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Issue 836 add lithium plating #1009

Issue 836 add lithium plating #1009

felipe-salinas commented May 21, 2020

valentinsulzer left a comment

valentinsulzer May 21, 2020

felipe-salinas May 25, 2020

valentinsulzer May 21, 2020

felipe-salinas May 25, 2020

valentinsulzer May 21, 2020

felipe-salinas May 25, 2020

valentinsulzer May 21, 2020

felipe-salinas May 25, 2020

valentinsulzer May 21, 2020

felipe-salinas May 25, 2020

valentinsulzer May 21, 2020

felipe-salinas May 25, 2020 •

edited

Loading

valentinsulzer May 21, 2020

felipe-salinas May 25, 2020

valentinsulzer May 21, 2020

felipe-salinas May 25, 2020

valentinsulzer May 21, 2020

felipe-salinas May 25, 2020

valentinsulzer left a comment

valentinsulzer May 27, 2020

valentinsulzer May 27, 2020

valentinsulzer May 27, 2020

valentinsulzer commented Jun 23, 2020

Issue 836 add lithium plating #1009

Issue 836 add lithium plating #1009

Conversation

felipe-salinas commented May 21, 2020

Description

Type of change

Key checklist:

Further checks:

valentinsulzer left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

felipe-salinas May 25, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

valentinsulzer left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

valentinsulzer commented Jun 23, 2020

felipe-salinas May 25, 2020 •

edited

Loading