Allow SEI layers choice

[lorenzofavaro]

Description

Currently, the default for SEI is one inner and one outer layer.
Continuing the work of #3457, this change is to include an option to have a single layer SEI model, rather than just the 2-layer version.

Closes #2333

Type of change

• New feature (non-breaking change which adds functionality)

Key checklist:

• No style issues: $ pre-commit run (or $ nox -s pre-commit)
• All tests pass: $ python run-tests.py --all (or $ nox -s tests)
• The documentation builds: $ python run-tests.py --doctest (or $ nox -s doctests)

[codecov]

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 99.58%. Comparing base (09ee982) to head (7a021e5).
Report is 3 commits behind head on develop.

Additional details and impacted files

@@           Coverage Diff            @@
##           develop    #3920   +/-   ##
========================================
  Coverage    99.58%   99.58%           
========================================
  Files          257      257           
  Lines        21245    21277   +32     
========================================
+ Hits         21157    21189   +32     
  Misses          88       88           

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

[kratman]

@lorenzofavaro This was being worked on by another person. Did you ask @GBlanka if they were still working on it? In general you don't want to take over the work of another developer unless they were not interested in continuing

[lorenzofavaro]

@lorenzofavaro This was being worked on by another person. Did you ask @GBlanka if they were still working on it? In general you don't want to take over the work of another developer unless they were not interested in continuing

You're right, I forgot to specify this in the PR description but I asked her about it on another platform (on Github she's no longer active) and she confirmed that she was no longer interested in working on the PR #3457.

Edit: She just closed her pull request.

[valentinsulzer]

Looks good to me except a few comments. I'm wondering whether it would be cleaner to have entirely separate classes for double-layer or single-layer SEIs. There are a lot of if statements now in sei_growth.py

[valentinsulzer]

I wouldn't add outer thickness in the single-layer case, only total thickness

[valentinsulzer]

or just call it "SEI thickness" (not outer or total)

[lorenzofavaro]

Right, we could rename it to just SEI thickness.
This renaming also apply to the other parameters in the single-layer case, right?
Ex:

f"X-averaged {domain} electrode outer {self.reaction_name}interfacial current density [A.m-2]"

would become:

f"X-averaged {domain} electrode {self.reaction_name}interfacial current density [A.m-2]"

[valentinsulzer]

electron-migration limited should still work with the single sei layer, we would just assume that the single layer is the inner layer

[lorenzofavaro]

we would just assume that the single layer is the inner layer

In the single-layer case we usually assume that the layer to be used is the outer layer right? Should we make an exception for this case and continue with the previous assumption in the remaining code?

[valentinsulzer]

If we just make it a separate file it will be easier as there won't be a need for an exception

[lorenzofavaro]

I'm wondering whether it would be cleaner to have entirely separate classes for double-layer or single-layer SEIs. There are a lot of if statements now in sei_growth.py

@tinosulzer Could we think of adding two classes SingleLayer and DoubleLayer in this way?

After that, update the function set_six_submodel in the base_lithium_ion_model module, so that it handles the correct initialization of the submodel, based on the double SEI layer option.

Edit: updated UML

[valentinsulzer]

NoSEI and ConstantSEI should only have the single-SEI case (as it's pointless to model a double-layer SEI if it's not there or constant). TotalSEI is an aggregator anyway so that should deal with both single and double cases. That only leaves SEIGrowth and DoubleLayerSEIGrowth. In fact we may want to put the "double" option inside the SEI option e.g. "solvent-diffusion limited" and "solvent-diffusion limited (double-layer)"

Btw, thanks for your work on this and sorry for changing the specs along the way, it just became clearer when thinking about it more

[valentinsulzer]

The other (simpler) option is to just get rid of double layer SEI, which is just confusing and hard to parameterize. Thoughts @rtimms @brosaplanella @DrSOKane ?

[rtimms]

I would happily remove the double layer stuff. If people really really want it they can implement as a custom submodel or basic model.

[brosaplanella]

I agree with Rob. In fact, I believe that most of the 2 layer models can be hacked into a single layer model just by adjusting the parameters.

Related to this, I am not sure if it would be better to split the various models into classes rather than have a single class with several if statements.

[valentinsulzer]

There's pros and cons to separating into different classes. I think a good middle ground is to have different submodel classes when the variables in submodel.rhs or submodels.algebraic change, and use if statements if we keep the same general structure but only vary the form of the equations. So in this case, keep it all in one submodel class

[valentinsulzer]

@lorenzofavaro the conclusion is let's just go ahead and get rid of the double SEI entirely. It should make the changes much cleaner

[brosaplanella]

Hi @lorenzofavaro! Any updates on this?