Negative half-cell models #3198

DrSOKane · 2023-07-27T16:40:11Z

Description

This pull request introduced negative half-cell models to PyBaMM, by means of three major changes:

The option "working electrode": ["both", "negative", "positive"] has been replaced with "half-cell": ["false", "true]. The positive electrode is always considered to be the working electrode, which has always been the case but is now made explicit.
Two new parameter sets - Ecker2015_graphite_halfcell and OKane2022_negative_halfcell - have been added to work with the new option.
The degradation mechanisms SEI, SEI on cracks and lithium plating can now be used on either electrode by specifying them as a 2-tuple, in the same way as is already done for some other options. Unlike for other options, however, passing only a single string instead of a tuple will put that behaviour on the negative electrode only.

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: $ pre-commit run (see CONTRIBUTING.md for how to set this up to run automatically when committing locally, in just two lines of code)
All tests pass: $ python run-tests.py --all
The documentation builds: $ python run-tests.py --doctest

You can run unit and doctests together 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

review-notebook-app · 2023-07-27T16:40:15Z

Check out this pull request on

See visual diffs & provide feedback on Jupyter Notebooks.

Powered by ReviewNB

codecov · 2023-07-27T17:01:39Z

Codecov Report

Attention: 1 lines in your changes are missing coverage. Please review.

Comparison is base (83d32ca) 99.58% compared to head (87da691) 99.58%.
Report is 2 commits behind head on develop.

Additional details and impacted files

@@            Coverage Diff            @@
##           develop    #3198    +/-   ##
=========================================
  Coverage    99.58%   99.58%            
=========================================
  Files          254      256     +2     
  Lines        19821    19998   +177     
=========================================
+ Hits         19738    19915   +177     
  Misses          83       83

Files	Coverage Δ
pybamm/expression_tree/averages.py	`100.00% <ø> (ø)`
...input/parameters/lithium_ion/Chen2020_composite.py	`100.00% <ø> (ø)`
...ameters/lithium_ion/Ecker2015_graphite_halfcell.py	`100.00% <100.00%> (ø)`
...rs/lithium_ion/OKane2022_graphite_SiOx_halfcell.py	`100.00% <100.00%> (ø)`
pybamm/input/parameters/lithium_ion/Xu2019.py	`100.00% <ø> (ø)`
...l_battery_models/lead_acid/base_lead_acid_model.py	`100.00% <100.00%> (ø)`
...models/full_battery_models/lithium_ion/Yang2017.py	`100.00% <ø> (ø)`
...ttery_models/lithium_ion/base_lithium_ion_model.py	`100.00% <100.00%> (ø)`
..._battery_models/lithium_ion/basic_dfn_half_cell.py	`100.00% <100.00%> (+1.11%)`	⬆️
...tery_models/lithium_ion/electrode_soh_half_cell.py	`100.00% <100.00%> (ø)`
... and 21 more

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

…issue-2730-graphite-halfcell

This reverts commit 4e49668.

brosaplanella

Looks great, thanks Simon!

brosaplanella · 2023-08-14T10:57:13Z

CHANGELOG.md

+- Option `working electrode` has been replaced with `half-cell`, which can only be `true` or `false`. The positive electrode is always considered to be the working electrode. For negative half-cells, lithium metal is now considered the negative electrode and the negative electrode material to be the positive electrode. ([#3198](https://github.com/pybamm-team/PyBaMM/pull/3198))
+- If `options["half-cell"] == "false"` and either `SEI`, `SEI on cracks` or `lithium plating` are not provided as tuples, they are automatically made into tuples. This directly modifies `extra_options`, not `default_options` to ensure the other changes `default_options` still happen when required. ([#3198](https://github.com/pybamm-team/PyBaMM/pull/3198))
+- Added option to use an empirical hysteresis model for the diffusivity and exchange-current density ([#3194](https://github.com/pybamm-team/PyBaMM/pull/3194))
+- PyBaMM now has optional dependencies that can be installed with the pattern `pip install pybamm[option]` e.g. `pybamm[plot]` ([#3044](https://github.com/pybamm-team/PyBaMM/pull/3044))
+- Double-layer capacity can now be provided as a function of temperature ([#3174](https://github.com/pybamm-team/PyBaMM/pull/3174))
+- `pybamm_install_jax` is deprecated. It is now replaced with `pip install pybamm[jax]` ([#3163](https://github.com/pybamm-team/PyBaMM/pull/3163))
+- PyBaMM now has optional dependencies that can be installed with the pattern `pip install pybamm[option]` e.g. `pybamm[plot]` ([#3044](https://github.com/pybamm-team/PyBaMM/pull/3044))


Suggested change

- Option `working electrode` has been replaced with `half-cell`, which can only be `true` or `false`. The positive electrode is always considered to be the working electrode. For negative half-cells, lithium metal is now considered the negative electrode and the negative electrode material to be the positive electrode. ([#3198](https://github.com/pybamm-team/PyBaMM/pull/3198))

- If `options["half-cell"] == "false"` and either `SEI`, `SEI on cracks` or `lithium plating` are not provided as tuples, they are automatically made into tuples. This directly modifies `extra_options`, not `default_options` to ensure the other changes `default_options` still happen when required. ([#3198](https://github.com/pybamm-team/PyBaMM/pull/3198))

- Added option to use an empirical hysteresis model for the diffusivity and exchange-current density ([#3194](https://github.com/pybamm-team/PyBaMM/pull/3194))

- PyBaMM now has optional dependencies that can be installed with the pattern `pip install pybamm[option]` e.g. `pybamm[plot]` ([#3044](https://github.com/pybamm-team/PyBaMM/pull/3044))

- Double-layer capacity can now be provided as a function of temperature ([#3174](https://github.com/pybamm-team/PyBaMM/pull/3174))

- `pybamm_install_jax` is deprecated. It is now replaced with `pip install pybamm[jax]` ([#3163](https://github.com/pybamm-team/PyBaMM/pull/3163))

- PyBaMM now has optional dependencies that can be installed with the pattern `pip install pybamm[option]` e.g. `pybamm[plot]` ([#3044](https://github.com/pybamm-team/PyBaMM/pull/3044))

- Option `working electrode` has been replaced with `half-cell`, which can only be `true` or `false`. The positive electrode is always considered to be the working electrode. For negative half-cells, lithium metal is now considered the negative electrode and the negative electrode material to be the positive electrode. ([#3198](https://github.com/pybamm-team/PyBaMM/pull/3198))

- If `options["half-cell"] == "false"` and either `SEI`, `SEI on cracks` or `lithium plating` are not provided as tuples, they are automatically made into tuples. This directly modifies `extra_options`, not `default_options` to ensure the other changes `default_options` still happen when required. ([#3198](https://github.com/pybamm-team/PyBaMM/pull/3198))

- Added option to use an empirical hysteresis model for the diffusivity and exchange-current density ([#3194](https://github.com/pybamm-team/PyBaMM/pull/3194))

- Double-layer capacity can now be provided as a function of temperature ([#3174](https://github.com/pybamm-team/PyBaMM/pull/3174))

- `pybamm_install_jax` is deprecated. It is now replaced with `pip install pybamm[jax]` ([#3163](https://github.com/pybamm-team/PyBaMM/pull/3163))

- PyBaMM now has optional dependencies that can be installed with the pattern `pip install pybamm[option]` e.g. `pybamm[plot]` ([#3044](https://github.com/pybamm-team/PyBaMM/pull/3044))

valentinsulzer

Looks good, thanks @DrSOKane !

A couple of comments:

Is a helpful error raised if the user tries to do {"working electrode": "positive"}?
It looks like the SEI variables now require a domain ("Negative electrode SEI thickness [m]" instead of "SEI thickness [m]"). Does that mean previous scripts with "SEI thickness [m]" will break?

rtimms · 2023-09-15T10:59:57Z

thanks @DrSOKane ! would be great to get this merged before the next release if you have time to respond to valentin's comments above

valentinsulzer

Upon further considering I think we should keep "working electrode": "positive" to avoid unnecessarily breaking people's code, and just error with "working electrode": "negative" with an error message that explains you always have to use "positive" and change the parameters. In future if we have a symmetric cell that can be "working electrode": "none"

DrSOKane · 2023-09-26T17:22:02Z

Hi @tinosulzer and @rtimms, I'm back from holiday and made this a priority.

All variables pertaining to the SEI, SEI on cracks and lithium plating submodels do indeed have domains now, and I've added this to the list of breaking changes.

Having thought about it, I agree that changing back to the working electrode syntax is the best way to go, so I've gone through every instance and changed it back. An OptionError is now raised if you attempt to set working electrode to be negative.

Finally, I've added an example notebook to show how to use the half-cell models.

brosaplanella · 2023-10-02T16:05:25Z

Happy to merge once the notebook conflicts are resolved

brosaplanella · 2023-10-03T11:37:09Z

@agriyakhetarpal do you know why the doc test is failing?

agriyakhetarpal · 2023-10-03T11:42:27Z

Yes, I think this is a bug that I have noted earlier (#3389). We should mark it at a higher priority now I guess

Edit: sorry, I read the logs too quickly. It looks like a different warning, I will debug and leave a review below.

DrSOKane · 2023-10-03T12:25:26Z

Yes, I think this is a bug that I have noted earlier (#3389). We should mark it at a higher priority now I guess

Edit: sorry, I read the logs too quickly. It looks like a different warning, I will debug and leave a review below.

The underline in parameter_sets.rst is to short to accommodate the name of the new parameter set, so I extended the underline. Should work now.

brosaplanella · 2023-10-03T14:09:16Z

Fantastic work @DrSOKane!

DrSOKane · 2023-10-03T14:13:47Z

Fantastic work @DrSOKane!

Thank you so much! This means a lot!

DrSOKane added 10 commits June 5, 2023 12:17

fixed tests

cf53ff1

Added graphite half-cell parameter files

78001e8

SEI on positive electrode should in theory work now

c4ee55d

Some debugging

7dfc5e3

Negative half-cell model working for non-composite cells

2cd694f

Reverted change to base_battery_model

bc1611f

Negative half-cell works with all 3 degradation mechanisms

2754ee6

fixed merge conflict

674661b

Some tests still failing

9a6176f

Fixed bug with variables names; all unit tests now pass

c73cca6

changelog

90ad2de

DrSOKane requested review from valentinsulzer and rtimms July 27, 2023 16:58

fixed merge conflict in changelog

31fd585

DrSOKane added 14 commits July 28, 2023 17:30

coverage

c05ac42

fixed merge conflicts

0f53811

Added integration tests

1db3221

Fixed parameter sets

a859d12

Integration tests pass

6e5886a

Fixed merge conflict in changelog

0714271

coverage

65c1841

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

f1b849e

…issue-2730-graphite-halfcell

Renamed test

13a6933

fixed merge conflict in changelog

8c2e550

Added 0% and 100% SoC limits to half-cell parameter sets

0d5c6c7

fixed example notebook

a9c3ec2

Fixed calendar ageing example

2e692d4

fixed cycling ageing example

d7bcdf4

DrSOKane added 2 commits August 16, 2023 15:10

Added new notebook to index.rst

4e49668

Revert "Added new notebook to index.rst"

102df06

This reverts commit 4e49668.

brosaplanella approved these changes Aug 21, 2023

View reviewed changes

valentinsulzer reviewed Sep 9, 2023

View reviewed changes

valentinsulzer requested changes Sep 21, 2023

View reviewed changes

DrSOKane mentioned this pull request Sep 25, 2023

[Bug]: Dead lithium not recorded in irreversible lithium plating model #3339

Closed

DrSOKane added 6 commits September 25, 2023 17:33

Changed 'half-cell' option back to 'working electrode'

e19cf5b

Raises special OptionError is working electrode is set to negative

b08dfa6

fixed merge conflicts from develop

a3c7612

whole_cell_domains is the right way around now!

fe918d5

moved example notebook to correct location

613a73f

Removed skip over 'working electrode' when checking validity of options

1c4569d

DrSOKane added 2 commits September 26, 2023 18:25

Added half-cell notebook to index

b5b8b87

Fixed size distribution test

52d486d

brosaplanella requested a review from valentinsulzer September 27, 2023 10:17

valentinsulzer approved these changes Oct 2, 2023

View reviewed changes

DrSOKane added 2 commits October 3, 2023 10:38

fixed merge conficts, except using-submodels.ipynb

aacae24

Fixed using-submodels.ipynb

836b777

Extended underline to accommodate new parameter set

87da691

brosaplanella merged commit e477c14 into pybamm-team:develop Oct 3, 2023
34 of 35 checks passed

arjxn-py mentioned this pull request Oct 7, 2023

Scheduled tests failing while running example tests on python 3.8 (Ubuntu) #3415

Closed

agriyakhetarpal mentioned this pull request Oct 25, 2023

JaxSolver fails when using GPU support with no input parameters #3423

Merged

8 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Negative half-cell models #3198

Negative half-cell models #3198

DrSOKane commented Jul 27, 2023

review-notebook-app bot commented Jul 27, 2023

codecov bot commented Jul 27, 2023 •

edited

Loading

brosaplanella left a comment

brosaplanella Aug 14, 2023

valentinsulzer left a comment

rtimms commented Sep 15, 2023

valentinsulzer left a comment •

edited

Loading

DrSOKane commented Sep 26, 2023

brosaplanella commented Oct 2, 2023

brosaplanella commented Oct 3, 2023

agriyakhetarpal commented Oct 3, 2023 •

edited

Loading

DrSOKane commented Oct 3, 2023 •

edited

Loading

brosaplanella commented Oct 3, 2023

DrSOKane commented Oct 3, 2023

Negative half-cell models #3198

Negative half-cell models #3198

Conversation

DrSOKane commented Jul 27, 2023

Description

Type of change

Key checklist:

Further checks:

review-notebook-app bot commented Jul 27, 2023

codecov bot commented Jul 27, 2023 • edited Loading

Codecov Report

brosaplanella left a comment

Choose a reason for hiding this comment

brosaplanella Aug 14, 2023

Choose a reason for hiding this comment

valentinsulzer left a comment

Choose a reason for hiding this comment

rtimms commented Sep 15, 2023

valentinsulzer left a comment • edited Loading

Choose a reason for hiding this comment

DrSOKane commented Sep 26, 2023

brosaplanella commented Oct 2, 2023

brosaplanella commented Oct 3, 2023

agriyakhetarpal commented Oct 3, 2023 • edited Loading

DrSOKane commented Oct 3, 2023 • edited Loading

brosaplanella commented Oct 3, 2023

DrSOKane commented Oct 3, 2023

codecov bot commented Jul 27, 2023 •

edited

Loading

valentinsulzer left a comment •

edited

Loading

agriyakhetarpal commented Oct 3, 2023 •

edited

Loading

DrSOKane commented Oct 3, 2023 •

edited

Loading