make SEI single layer only #4470
Conversation
valentinsulzer
requested review from
brosaplanella,
DrSOKane,
rtimms,
TomTranter and
kratman
as code owners
|
We should probably merge this one before #4394 (tagging @kawaMANMI so he is aware of this PR). |
There was a problem hiding this comment.
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.
|
|
||
| Returns | ||
| ------- | ||
| variables : dict | ||
| The variables which can be derived from the SEI currents. | ||
| """ | ||
| domain, Domain = self.domain_Domain | ||
| j_inner_av = pybamm.x_average(j_inner) | ||
| j_outer_av = pybamm.x_average(j_outer) | ||
| j_sei = j_inner + j_outer | ||
|
|
There was a problem hiding this comment.
Why is there no jsei_av variable?
| @@ -134,16 +123,16 @@ def get_coupled_variables(self, variables): | |||
| elif SEI_option == "electron-migration limited": | |||
| # Scott Marquis thesis (eq. 5.94) | |||
| eta_inner = delta_phi - phase_param.U_inner | |||
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## develop #4470 +/- ##
===========================================
- Coverage 99.42% 99.42% -0.01%
===========================================
Files 297 299 +2
Lines 22685 22673 -12
===========================================
- Hits 22554 22542 -12
Misses 131 131 ☔ View full report in Codecov by Sentry. |
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.
Key checklist:
$ 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)$ python run-tests.py --all(or$ nox -s tests)$ 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: