Yang2017 Model #1398

KennethNwanoro · 2021-02-25T16:05:46Z

Description

The Yang2017 model couples irreversible lithium plating, SEI growth and change in porosity which produces a transition from linear to nonlinear degradation pattern of lithium ion battery over extended cycles.

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

brosaplanella · 2021-02-25T16:21:17Z

Hi Kenneth, thanks for the PR! In order to check everything is in order, can you please merge the most recent version of the develop branch and push again? Otherwise, the automatic tests don't run.

KennethNwanoro · 2021-02-26T10:44:34Z

Hi Ferran,

I have updated the branch. Could you please check again.
I am seeing warning about conflict with lithium_ion_parameters.py file. I have checked but don't see any issues.
Many thanks.

brosaplanella · 2021-02-26T13:22:24Z

There are some conflicts on the file still to be resolved. You can either resolve them on the web editor (clicking the Resolve conflicts button) or on your local editor (most of them have in-built functionality to assist with that). It also seems that the develop branch you merged is not the most recent one, so you first need to update to the most recent version of the develop branch. Let me know if I can help.

brosaplanella · 2021-02-26T13:33:14Z

Now the tests run, so the first thing to fix is the style. Here you can find more information about it. You can install the package black which automatically formats your code.

…to develop

KennethNwanoro · 2021-02-26T14:08:40Z

Hi Ferran, I have done this. Could please check again. Thanks

valentinsulzer

Thanks @KennethNwanoro , looking pretty good, just a few things to change.
In terms of tests to add, we need:

tests for the lithium plating porosity change, in this file, e.g.

def test_well_posed_reversible_plating_with_porosity(self):
        options = {"lithium plating": "reversible", "lithium plating porosity change": "true"}
        model = pybamm.lithium_ion.DFN(options)
        model.check_well_posedness()

and then similarly in this file, and also equivalent tests for the SPM and SPMe

a test for the Yang2017 model in a new file, for example

def test_basic_processing(self):
        model = pybamm.lithium_ion.Yang2017()
        modeltest = tests.StandardModelTest(model)
        modeltest.test_all()

pybamm/models/full_battery_models/lithium_ion/basic_dfn_half_cell.py

...thium-ion/lithium_platings/yang2017_Li_plating/plating_exchange_current_density_OKane2020.py

...hium-ion/negative_electrodes/graphite_Yang2017/plating_exchange_current_density_OKane2020.py

pybamm/models/full_battery_models/base_battery_model.py

pybamm/models/full_battery_models/lithium_ion/basic_dfn_half_cell.py

…to develop

brosaplanella

Hi Kenneth! Looks good. Just a few comments from me, which should help with making the tests pass.

brosaplanella · 2021-03-08T15:29:07Z

pybamm/models/full_battery_models/lithium_ion/spme.py

+        ):
+            self.submodels["porosity"] = pybamm.porosity.LeadingOrder(
+                self.param, self.options
+            )
        if self.options["SEI porosity change"] == "false":
            self.submodels["porosity"] = pybamm.porosity.Constant(self.param)


You need to pass self.options as second argument (that is what causes most of the tests to fail).

brosaplanella · 2021-03-08T15:29:17Z

pybamm/models/full_battery_models/lithium_ion/spm.py

+        ):
+            self.submodels["porosity"] = pybamm.porosity.LeadingOrder(
+                self.param, self.options
+            )
        if self.options["SEI porosity change"] == "false":
            self.submodels["porosity"] = pybamm.porosity.Constant(self.param)


You need to pass self.options as second argument (that is what causes most of the tests to fail).

brosaplanella · 2021-03-08T15:29:22Z

pybamm/models/full_battery_models/lithium_ion/dfn.py

+            self.options["sei porosity change"] == "true"
+            or self.options["lithium plating porosity change"] == "true"
+        ):
+            self.submodels["porosity"] = pybamm.porosity.Full(self.param, self.options)
        if self.options["SEI porosity change"] == "false":
            self.submodels["porosity"] = pybamm.porosity.Constant(self.param)


You need to pass self.options as second argument (that is what causes most of the tests to fail).

...thium-ion/lithium_platings/yang2017_Li_plating/plating_exchange_current_density_OKane2020.py

brosaplanella · 2021-03-08T16:38:42Z

pybamm/input/parameters/lithium-ion/negative_electrodes/graphite_Yang2017/README.md

@@ -0,0 +1,12 @@
+# Graphite negative electrode parameters


Are we using the Ecker parameter set for Yang? If so, we don't need to define it again. Otherwise, the Yang paper should be listed in the references here.

brosaplanella · 2021-03-08T16:40:19Z

pybamm/input/parameters/lithium-ion/seis/yang2017_sei/README.md

@@ -0,0 +1,11 @@
+# SEI parameters


Same comment as with graphite

valentinsulzer

Looks great, some comments:

check plating function names / duplication
move the "electrolytes" folder out of the "cells"
check whether we need to redefine the Ecker parameter set or if we can just use the existing one

...thium-ion/lithium_platings/yang2017_Li_plating/plating_exchange_current_density_OKane2020.py

...hium-ion/negative_electrodes/graphite_Yang2017/plating_exchange_current_density_OKane2020.py

pybamm/models/full_battery_models/base_battery_model.py

pybamm/models/full_battery_models/lithium_ion/basic_dfn_half_cell.py

KennethNwanoro · 2021-03-19T09:57:49Z

@tinosulzer
Response to comment 1:
The lithium plating exchange current density for Yang2017 is a constant, I initially used the OKane function for consistency but since that is a bit confusing, I have reverted to using a constant value without using a FunctionParameter.

Response to comment 2:
I have removed this folder in the cells folder (strange how that got that, I might have moved it unknowingly.

Response to comment 3:
Here is my response to Ferran's comment about this.
Most of the parameters (OCPs, coefficients and thermal parameters) used in the Yang2017 were not listed in that paper. I have adapted and changed some parameter values in the Ecker parameter set that were given in Yang2017 paper. I think it is ok to have Yang2017 parameter set defined now even if some parameters values were from Ecker (I have referenced Ecker in the Yang2017 parameter set) so that when the full parameters from that paper is obtained (I plan to contact them regarding this), it will be easier to just change them in the already defined Yang2017 parameter set.

Many thanks

brosaplanella

Looking good! Just a few minor comments (see the suggested changes). There is also a file called python which should be deleted from the commit.

pybamm/CITATIONS.txt

brosaplanella · 2021-03-18T19:33:09Z

...-ion/cells/electrolytes/lipf6_EC_DMC_1_1_Landesfeind2019/electrolyte_base_Landesfeind2019.py

@@ -0,0 +1,155 @@
+from pybamm import exp, sqrt


I think what happened with the electrolyte parameters is that they were accidentally moved into cells. Remove the folder pybamm/input/parameters/lithium-ion/cells/electrolytes/

...thium-ion/lithium_platings/yang2017_Li_plating/plating_exchange_current_density_OKane2020.py

pybamm/models/submodels/interface/lithium_plating/irreversible_plating.py

pybamm/models/submodels/interface/lithium_plating/reversible_plating.py

brosaplanella · 2021-03-19T11:00:59Z

tests/unit/test_models/test_submodels/test_porosity/test_full_reaction_driven_porosity.py

-        param = pybamm.LeadAcidParameters()
+        # param = pybamm.LeadAcidParameters()


Same as with the leading order reaction driven porosity.

pybamm/input/parameters/lithium-ion/lithium_platings/yang2017_Li_plating/parameters.csv

pybamm/models/submodels/porosity/base_porosity.py

pybamm/models/submodels/porosity/full_reaction_driven_porosity.py

pybamm/models/submodels/porosity/leading_reaction_driven_porosity.py

…ity.py Co-authored-by: Ferran Brosa Planella <Ferran.Brosa-Planella@warwick.ac.uk>

Co-authored-by: Ferran Brosa Planella <Ferran.Brosa-Planella@warwick.ac.uk>

…Li_plating/parameters.csv Co-authored-by: Ferran Brosa Planella <Ferran.Brosa-Planella@warwick.ac.uk>

…thium_ion/test_spm.py Co-authored-by: Ferran Brosa Planella <Ferran.Brosa-Planella@warwick.ac.uk>

…lating.py Co-authored-by: Ferran Brosa Planella <Ferran.Brosa-Planella@warwick.ac.uk>

…thium_ion/test_yang2017.py Co-authored-by: Ferran Brosa Planella <Ferran.Brosa-Planella@warwick.ac.uk>

…ery_model.py Co-authored-by: Ferran Brosa Planella <Ferran.Brosa-Planella@warwick.ac.uk>

…_plating.py Co-authored-by: Ferran Brosa Planella <Ferran.Brosa-Planella@warwick.ac.uk>

Co-authored-by: Ferran Brosa Planella <Ferran.Brosa-Planella@warwick.ac.uk>

brosaplanella

Looks good to me, great work! Let's see if @tinosulzer has any comments and it should be good to merge.

You should add a line on CHANGELOG.md under new features (note that they are sorted in order of descending PR number, so the comment should go between PR #1408 and #1370).

valentinsulzer

Thanks, looks good to me now! The final thing to do is to update the changelog, and also add the Yang2017 model to the docs (see dfn.rst for an example). Hopefully we can get the real parameters from the authors so that we can replicate their results exactly!

valentinsulzer · 2021-03-19T13:19:02Z

tests/unit/test_models/test_submodels/test_porosity/test_leading_reaction_driven_porosity.py

-        param = pybamm.LeadAcidParameters()
+        # param = pybamm.LeadAcidParameters()
+        param = pybamm.LithiumIonParameters()


It's probably why we miss the coverage earlier but it's fine - need to reformat these porosity models anyway

valentinsulzer · 2021-03-19T13:25:11Z

@all-contributors add @KennethNwanoro for code, tests

allcontributors · 2021-03-19T13:25:20Z

@tinosulzer

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

docs/source/models/lithium_ion/yang2017.rst

Co-authored-by: Ferran Brosa Planella <Ferran.Brosa-Planella@warwick.ac.uk>

KennethNwanoro and others added 6 commits February 4, 2021 18:20

Create Yang2017

d267aa3

Create parameters.csv

28679bd

Rename Yang2017 to Yang2017.py

55821b4

Check issues associated with calling Yang2017

38df929

Corrections on Yang2017

0e3a63e

Yang2017 corrections

7d91314

Corrections

3cd1609

KennethNwanoro closed this Feb 26, 2021

KennethNwanoro reopened this Feb 26, 2021

Merge branch 'develop' into develop

6d7cab9

KennethNwanoro added 2 commits February 26, 2021 13:51

Formatted

a14bf32

Merge branch 'develop' of https://github.com/KennethNwanoro/PyBaMM in…

94e9026

…to develop

KennethNwanoro added 2 commits February 26, 2021 16:50

Spacing corrections

e53004b

Spacing corrections

2e9adfb

valentinsulzer requested changes Mar 1, 2021

View reviewed changes

KennethNwanoro and others added 5 commits March 1, 2021 18:26

Tests

04a3a35

Merge branch 'develop' into develop

17b8d3e

Tests

79298fb

Changes for tests

d660020

Merge branch 'develop' of https://github.com/KennethNwanoro/PyBaMM in…

033e7cb

…to develop

brosaplanella requested changes Mar 8, 2021

View reviewed changes

KennethNwanoro and others added 4 commits March 9, 2021 11:14

Corrections for tests

7036d54

Changes for integration tests

f5dd4ea

Merge branch 'develop' into develop

a724f7a

Update base_battery_model.py

1e1806e

KennethNwanoro and others added 4 commits March 18, 2021 14:01

Changes for tests

38e262b

Update README.md

3b1811e

Changes for tests

386e135

Update README.md

a331bad

valentinsulzer requested changes Mar 18, 2021

View reviewed changes

Changes for tests

1f27f3b

brosaplanella requested changes Mar 19, 2021

View reviewed changes

KennethNwanoro and others added 11 commits March 19, 2021 11:22

Update pybamm/models/submodels/porosity/leading_reaction_driven_poros…

b724272

…ity.py Co-authored-by: Ferran Brosa Planella <Ferran.Brosa-Planella@warwick.ac.uk>

Update pybamm/models/submodels/porosity/full_reaction_driven_porosity.py

b8e296e

Co-authored-by: Ferran Brosa Planella <Ferran.Brosa-Planella@warwick.ac.uk>

Update pybamm/models/submodels/porosity/base_porosity.py

5a2c67c

Co-authored-by: Ferran Brosa Planella <Ferran.Brosa-Planella@warwick.ac.uk>

Update pybamm/input/parameters/lithium-ion/lithium_platings/yang2017_…

a13b035

…Li_plating/parameters.csv Co-authored-by: Ferran Brosa Planella <Ferran.Brosa-Planella@warwick.ac.uk>

Update tests/integration/test_models/test_full_battery_models/test_li…

651d21f

…thium_ion/test_spm.py Co-authored-by: Ferran Brosa Planella <Ferran.Brosa-Planella@warwick.ac.uk>

Update pybamm/models/submodels/interface/lithium_plating/reversible_p…

499805c

…lating.py Co-authored-by: Ferran Brosa Planella <Ferran.Brosa-Planella@warwick.ac.uk>

Update tests/integration/test_models/test_full_battery_models/test_li…

f9b5370

…thium_ion/test_yang2017.py Co-authored-by: Ferran Brosa Planella <Ferran.Brosa-Planella@warwick.ac.uk>

Update tests/unit/test_models/test_full_battery_models/test_base_batt…

149c365

…ery_model.py Co-authored-by: Ferran Brosa Planella <Ferran.Brosa-Planella@warwick.ac.uk>

Update pybamm/models/submodels/interface/lithium_plating/irreversible…

731048e

…_plating.py Co-authored-by: Ferran Brosa Planella <Ferran.Brosa-Planella@warwick.ac.uk>

Update pybamm/CITATIONS.txt

acd47ac

Co-authored-by: Ferran Brosa Planella <Ferran.Brosa-Planella@warwick.ac.uk>

Delete python

276b1c6

KennethNwanoro requested review from brosaplanella and valentinsulzer March 19, 2021 12:38

brosaplanella approved these changes Mar 19, 2021

View reviewed changes

valentinsulzer approved these changes Mar 19, 2021

View reviewed changes

allcontributors bot mentioned this pull request Mar 19, 2021

docs: add KennethNwanoro as a contributor #1438

Merged

Changes for changelog.md

c7154e5

brosaplanella reviewed Mar 19, 2021

View reviewed changes

docs/source/models/lithium_ion/yang2017.rst Outdated Show resolved Hide resolved

Update docs/source/models/lithium_ion/yang2017.rst

c092252

Co-authored-by: Ferran Brosa Planella <Ferran.Brosa-Planella@warwick.ac.uk>

brosaplanella merged commit ed6e01a into pybamm-team:develop Mar 20, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Yang2017 Model #1398

Yang2017 Model #1398

KennethNwanoro commented Feb 25, 2021 •

edited

Loading

brosaplanella commented Feb 25, 2021

KennethNwanoro commented Feb 26, 2021

brosaplanella commented Feb 26, 2021

brosaplanella commented Feb 26, 2021

KennethNwanoro commented Feb 26, 2021

valentinsulzer left a comment

brosaplanella left a comment

brosaplanella Mar 8, 2021

brosaplanella Mar 8, 2021

brosaplanella Mar 8, 2021

brosaplanella Mar 8, 2021

brosaplanella Mar 8, 2021

valentinsulzer left a comment

KennethNwanoro commented Mar 19, 2021

brosaplanella left a comment

brosaplanella Mar 18, 2021

brosaplanella Mar 19, 2021

brosaplanella left a comment

valentinsulzer left a comment •

edited

Loading

valentinsulzer Mar 19, 2021

valentinsulzer commented Mar 19, 2021

allcontributors bot commented Mar 19, 2021

		param = pybamm.LeadAcidParameters()
		# param = pybamm.LeadAcidParameters()

Yang2017 Model #1398

Yang2017 Model #1398

Conversation

KennethNwanoro commented Feb 25, 2021 • edited Loading

Description

Type of change

Key checklist:

Further checks:

brosaplanella commented Feb 25, 2021

KennethNwanoro commented Feb 26, 2021

brosaplanella commented Feb 26, 2021

brosaplanella commented Feb 26, 2021

KennethNwanoro commented Feb 26, 2021

valentinsulzer left a comment

Choose a reason for hiding this comment

brosaplanella 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

valentinsulzer left a comment

Choose a reason for hiding this comment

KennethNwanoro commented Mar 19, 2021

brosaplanella left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

brosaplanella left a comment

Choose a reason for hiding this comment

valentinsulzer left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

valentinsulzer commented Mar 19, 2021

allcontributors bot commented Mar 19, 2021

KennethNwanoro commented Feb 25, 2021 •

edited

Loading

valentinsulzer left a comment •

edited

Loading