Skip to content
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

Change fundamental SEI variable from thickness to concentration: attempt 2 #4869

Open
wants to merge 25 commits into
base: develop
Choose a base branch
from

Conversation

DrSOKane
Copy link
Contributor

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

@DrSOKane DrSOKane requested a review from a team as a code owner February 21, 2025 19:04
Copy link

Check out this pull request on  ReviewNB

See visual diffs & provide feedback on Jupyter Notebooks.


Powered by ReviewNB

@DrSOKane
Copy link
Contributor Author

DrSOKane commented Feb 27, 2025

The reason the tests are failing is because none of the half-cell parameter sets have a value for "Initial SEI concentration [mol.m-2]" on the lithium metal electrode. Liu and Lu (2017) propose values for this and other SEI parameters on Li metal electrodes. I went ahead using those and added a citation for Liu and Lu to all half-cell parameter sets.

Copy link

codecov bot commented Feb 27, 2025

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 98.71%. Comparing base (edbec6c) to head (7962766).
Report is 1 commits behind head on develop.

Additional details and impacted files
@@           Coverage Diff            @@
##           develop    #4869   +/-   ##
========================================
  Coverage    98.71%   98.71%           
========================================
  Files          304      305    +1     
  Lines        23495    23500    +5     
========================================
+ Hits         23192    23198    +6     
+ Misses         303      302    -1     

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

@rtimms
Copy link
Contributor

rtimms commented Feb 28, 2025

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.

@DrSOKane
Copy link
Contributor Author

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!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Bug]: LLI due to SEI incorrect if loss of active material is enabled
2 participants