-
-
Notifications
You must be signed in to change notification settings - Fork 689
Change fundamental SEI variable from thickness to concentration: attempt 2 #4869
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: develop
Are you sure you want to change the base?
Conversation
…SEI-variable-change
…l.m-3]' in all parameter sets
…SEI-variable-change
Check out this pull request on See visual diffs & provide feedback on Jupyter Notebooks. Powered by ReviewNB |
The reason the tests are failing is because none of the half-cell parameter sets have a value for |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## develop #4869 +/- ##
===========================================
+ Coverage 98.77% 98.79% +0.02%
===========================================
Files 320 321 +1
Lines 27111 27123 +12
===========================================
+ Hits 26779 26797 +18
+ Misses 332 326 -6 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
In general, I am against adding parameters that aren't in the original paper, but since we already added a bunch of extra degradation parameters in these files it is OK to add this once too. Going forward we should be more strict on this. |
The only difference is that Liu and Lu use an initial thickness of 10 nm as opposed to 5 nm for Safari et al. I wanted to check if the initial thickness was substantially different for planar electrodes, and it turns out it's not. So I could calculate an interfacial concentration from 5 nm instead of 10 nm, then remove the extra reference. I don't know why the notebook is failing, it works fine when I run it! |
…SEI-variable-change
…SEI-variable-change
…SEI-variable-change
@DrSOKane is this ready to be reviewed? |
Yes please! All the commits since removing the reference to Liu and Lu are just updated from develop branch |
CHANGELOG.md
Outdated
- Changed fundamental variable for all SEI models from thickness to concentration ([#4869](https://github.com/pybamm-team/PyBaMM/pull/4869)) | ||
- Replaced parameter `"Initial SEI thickness [m]"` with `"Initial SEI concentration [mol.m-3]"` ([#4869](https://github.com/pybamm-team/PyBaMM/pull/4869)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just a note for later, this needs to be moved up to a current release
…SEI-variable-change
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
thanks @DrSOKane looks good, just a few comments
|
||
gamma_0 = pybamm.Parameter("Dead lithium decay constant [s-1]") | ||
L_sei_0 = pybamm.Parameter("Initial SEI thickness [m]") | ||
L_sei_0 = pybamm.Scalar(5e-9) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this consistent if the initial SEI concentration changes, or should it be derived from that?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's a tricky one, because the thickness and concentration are coupled via the surface area to volume ratio, which is different for every cell. For every cell except OKane2022, I derive the initial concentration to be consistent with that in the original Safari et al. (2009) paper. For OKane2022, the SEI is explicitly included, so I chose a value that results in a thickness of 5 nm exactly.
Safari et al. do not consider dead lithium, so I thought it more appropriate to stick with the value from O'Kane et al. Unfortunately, my dead lithium model has never been validated, so a factor of two difference is really not going to matter! The function is only there at all as proof of concept for pure graphite, as opposed to graphite+SiOx.
|
||
gamma_0 = pybamm.Parameter("Dead lithium decay constant [s-1]") | ||
L_sei_0 = pybamm.Parameter("Initial SEI thickness [m]") | ||
L_sei_0 = pybamm.Scalar(5e-9) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
as above
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It was difficult deciding whether to make the initial condition a concentration or thickness. Most of the academic literature reports thickness, with Li et al. 2025 being a notable exception. But for industry, concentration will be better if - and this is a big if - the formation losses are known, because the formation loss is the initial value.
…SEI-variable-change
…SEI-variable-change
Hi everyone, I tried using the new framework with composite parameter sets and found a problem: how do you distribute the SEI between the two phases? Most PyBaMM models tend towards the two phases having equal SEI thickness in the long term, so I decided that it makes more sense to revert to giving the initial condition as thickness, but keep concentration as the fundamental variable on the back end. To make up for this, I added a notebook that explains how to update PyBaMM parameter sets to account for any formation or storage losses a user may have measured. |
Description
Previously, lithium was not conserved if both SEI growth and loss of active material were enabled. To fix this, the fundamental variable to be solved for is now SEI concentration, not thickness. This has the added bonus of simplifying the SEI on cracks model.
I closed #3171 and made a new one simply because PyBaMM has changed so much since then!
One major difference between #3171 and this PR is that this time around, I also deleted the parameter
"Initial SEI thickness [m]"
and replaced it with"Initial SEI concentration [mol.m-3]"
. I did this partly to make the initial conditions less complicated, and partly because it's easier to measure the amount of lithium consumed by formation than it is to measure or even estimate the initial thickness. I assumed a value of 38.34 [mol.m-3], based on Ramadass et al. (2004) and Safari et al. (2009). The exception is"OKane2022"
, for which I used the exact value (to 4 significant figures anyway!)Fixes #3006