-
-
Notifications
You must be signed in to change notification settings - Fork 590
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
Fixed domain mismatch bug when using lithium plating with SPM #4844
Conversation
…lithium-plating-SPM
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## develop #4844 +/- ##
========================================
Coverage 98.70% 98.70%
========================================
Files 304 304
Lines 23452 23456 +4
========================================
+ Hits 23149 23153 +4
Misses 303 303 ☔ View full report in Codecov by Sentry. |
f"{Domain} {phase_name}lithium plating thickness [m]": L_plated_Li, | ||
f"X-averaged {domain} {phase_name} lithium plating thickness " | ||
f"X-averaged {domain} {phase_name}lithium plating thickness " |
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! Can you also add the relevant “Volume-averaged…” variables?
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.
That would be consistent with the definition of "volume-averaged" in the thermal submodel. However, in the particle submodel, "volume-averaged" is used to denoted the x-average of the r-averaged concentration. What term should be used instead?
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.
I think it is ok, since plated Lithium is on the surface (i.e. doesn’t depend on r
).
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.
OK, I did that. Now I can start on the variable change for the SEI. That PR is so old I will probably just close it and start a new one.
Description
Do you remember the bug I had when trying to change the fundamental variable for SEI from thickness to concentration? The bug didn't originate there. It originated in the lithium plating model, which I copy-pasted the code from. And I was able to fix it by performing a yz-average over the relevant variable.
I also edited the cycling ageing example to prove the fix works.