make SEI single layer only

[valentinsulzer]

Description

Remove the double-layer SEI and make it single layer only
This PR falls in the category of "breaking changes to the parameter names" so let's discuss how to handle this in the developer meeting - draft only for now

Fixes #2333
Replaces #3920

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 (or $ nox -s pre-commit) (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 (or $ nox -s tests)
• The documentation builds: $ python run-tests.py --doctest (or $ nox -s doctests)

You can run integration tests, unit tests, and doctests together at once, using $ python run-tests.py --quick (or $ nox -s quick).

Further checks:

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

[brosaplanella]

We should probably merge this one before #4394 (tagging @kawaMANMI so he is aware of this PR).

[DrSOKane]

I agree with this change. As we were able to show in our joint paper, the effect of the two-layer structure on SEI growth rate is dwarfed by that of altering the parameters. The only exception I can think of is if there are two different mechanisms occuring simultaneously. In-situ measurements of SEI growth and kinetic Monte-Carlo simulations both support such a scenario. I'm not aware of it being incorporated into a continuum-scale model yet, although it has been mooted. It would almost certainly require a separate class if it were ever to be implemented in PyBaMM.

I see no reason why _get_standard_thickness_variables and _get_standard_total_thickness_variables cannot be merged into a single function.

[DrSOKane]

Why is there no jsei_av variable?

[brosaplanella]

It is defined later on, in lines 248-253

[DrSOKane]

I noticed U_outer doesn't get used anywhere. Deleting U_inner and U_outer and just having U_sei would reduce the total number of parameters, but I'm aware that U_inner and U_sei are for different mechanisms

[brosaplanella]

Yes, I spotted that too. I guess the main question is, if we switch to just U_sei, which parameter value should we use? I believe at the moment one is set at 0.1 V and the other at 0.8 V.

UPDATE: just realised the O'Kane parameter sets assume inner SEI fraction to be 0, so I will set the U_SEI to 0.8 V

UPDATE 2: I defined "SEI open-circuit potential [V]" to 0.4 V as that was the current state in almost all parameter sets (just got rid of the inner and outer ones). For OKane2022, I still kept it at 0.8 as the inner SEI fraction was set to 0. Where examples where failing, I manually set the parameter back to what it was.

[codecov]

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 99.25%. Comparing base (5b1ef70) to head (bf2342a).
Report is 2 commits behind head on develop.

Additional details and impacted files

@@             Coverage Diff             @@
##           develop    #4470      +/-   ##
===========================================
- Coverage    99.25%   99.25%   -0.01%     
===========================================
  Files          302      302              
  Lines        22906    22838      -68     
===========================================
- Hits         22735    22667      -68     
  Misses         171      171              

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

---

🚨 Try these New Features:

• Flaky Tests Detection - Detect and resolve failed and flaky tests

[review-notebook-app]

Check out this pull request on 

See visual diffs & provide feedback on Jupyter Notebooks.

---

Powered by ReviewNB

[DrSOKane]

Hi Ferran, thank you very much for considering my feedback. I have two more minor edits. However, a broader discussion around the three different SEI open-circuit potentials, and what they used to do before being unified, will help avoid any present and future misunderstandings.

• U_sei_inner is used by the "electron-migration limited" model, but no other. Substituting U_sei_inner with U_sei in line 124 of base_sei.py will significantly alter the results of this model, unless the user changes the value U_sei accordingly. However, according to @Scottmar93 thesis, this model is not widely used, so it's a sacrifice worth making.
• U_sei is used by the "reaction limited" and "ec reaction limited" models. This PR does not change this.
• U_sei_outer was not used by any model.

There is therefore no basis for changing the value of U_sei in the OKane2022 parameter set. Please revert this change.

Also, I tried running half_cell.ipynb with your proposed value of U_sei = 0.1 and the results are massively different. Please revert this change as well.

I would apologize for the wall of text but I know you enjoy a good SEI discussion even more than I do!

[brosaplanella]

Hi @DrSOKane! Thanks for the comments, I hadn't realised that U_outer was actually never used and I also didn't realised that your parameter sets were used with models that only use U_sei. I thought they used the inner and outer models, hence my confusion.

I have also noticed that diffusivity is defined to 2.5000000000000002e-22, which probably resulted from copy pasting from some other code. I will chop them to 2 significant digits.

[kratman]

I made this a draft to keep it from being merged accidentally

[kratman]

@valentinsulzer Removing the skip-release tag now

[brosaplanella]

I have now fixed the conflicts, so this should be ready to merge. Because @valentinsulzer opened the PR he can't officially review it, but can you take a look at the changes I made and let me know if you are happy with it?