Issue 3339 dead lithium#3485
Conversation
|
Check out this pull request on See visual diffs & provide feedback on Jupyter Notebooks. Powered by ReviewNB |
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## develop #3485 +/- ##
========================================
Coverage 99.58% 99.58%
========================================
Files 256 256
Lines 20119 20126 +7
========================================
+ Hits 20036 20043 +7
Misses 83 83 ☔ View full report in Codecov by Sentry. |
brosaplanella
left a comment
brosaplanella
left a comment
There was a problem hiding this comment.
Looks good Simon, thanks! Just a couple of small comments.
There was a problem hiding this comment.
I have noticed that both Plated lithium capacity and Intercalated lithium capacity have units of Ah rather than A.h (that's a legacy thing, not introduced in this issue). Could you change it to A.h so it is consistent with the other variable names?
|
|
||
| ## Bug fixes | ||
|
|
||
| - The irreversible plating model now increments `f"{Domain} dead lithium concentration [mol.m-3]"`, not `f"{Domain} lithium plating concentration [mol.m-3]"` as it did previously. ([#3485](https://github.com/pybamm-team/PyBaMM/pull/3485)) |
There was a problem hiding this comment.
Can you move this to the unreleased version?
|
Sorry for the delay on getting back to this. CHANGELOG needs fixing as we updated develop, happy for this to be merged once that is fixed. |
Ruff is now failing, saying that |
|
I think @Saransh-cpp fixed it in #3519 |
|
Yes, please ignore the example notebooks and ruff failure. I will look into it this weekend. |
Saransh-cpp
left a comment
Saransh-cpp
left a comment
There was a problem hiding this comment.
Sorry for all the trouble, @DrSOKane. Everything should work now once you merge in develop and resolve the conflicts!
brosaplanella
left a comment
brosaplanella
left a comment
There was a problem hiding this comment.
Just a small change in the CHANGELOG as an entry is duplicate (see the suggestion).
| - Fixed bug that made identical Experiment steps with different end times crash ([#3516](https://github.com/pybamm-team/PyBaMM/pull/3516)) | ||
| - Fixed bug in calculation of theoretical energy that made it very slow ([#3506](https://github.com/pybamm-team/PyBaMM/pull/3506)) | ||
| - The irreversible plating model now increments `f"{Domain} dead lithium concentration [mol.m-3]"`, not `f"{Domain} lithium plating concentration [mol.m-3]"` as it did previously. ([#3485](https://github.com/pybamm-team/PyBaMM/pull/3485)) | ||
| - Fixed a bug where the JaxSolver would fails when using GPU support with no input parameters ([#3423](https://github.com/pybamm-team/PyBaMM/pull/3423)) |
There was a problem hiding this comment.
| - Fixed a bug where the JaxSolver would fails when using GPU support with no input parameters ([#3423](https://github.com/pybamm-team/PyBaMM/pull/3423)) |
There was a problem hiding this comment.
How is this a dupicate?
There was a problem hiding this comment.
That one made it in the release, see line 29 of the CHANGELOG
|
Thanks Simon, and sorry for all the back and forth! |
* fixed tests * Added graphite half-cell parameter files * Revert "Added graphite half-cell parameter files" This reverts commit 78001e8. * Revert "fixed tests" This reverts commit cf53ff1. * ruff * changelog * coverage * Fixed minor error in example notebook * Removed duplicate entry from changelog
3 participants
Description
The irreversible plating model now increases the
f"{Domain} dead lithium concentration [mol.m-3]"variable, not thef"{Domain} lithium plating concentration [mol.m-3]"variable as it did previously. Thef"Loss of lithium to {domain} plating [mol]"andf"Loss of capacity to {domain} plating [A.h]"variables are unchanged because they include both plated lithium and dead lithium.Fixes #3339