I3769 kolzenberg2020 (Another option for the SEI growth)#4394
I3769 kolzenberg2020 (Another option for the SEI growth)#4394kratman merged 53 commits intopybamm-team:developfrom
Conversation
|
Great, thanks @kawaMANMI! Will take a look later :) |
brosaplanella
left a comment
There was a problem hiding this comment.
Hi Kawa! Nice work, getting there. I have made some suggestions, including one that will hopefully make tests pass. We can address coverage once tests pass.
…yBaMM into i3769_Kolzenberg2020
14a4d8d to
2ac3484
Compare
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## develop #4394 +/- ##
===========================================
- Coverage 99.22% 99.18% -0.04%
===========================================
Files 302 302
Lines 22821 22840 +19
===========================================
+ Hits 22643 22654 +11
- Misses 178 186 +8 ☔ View full report in Codecov by Sentry. 🚨 Try these New Features:
|
|
Hi Kawa! The tests pass now, well done. The last thing is to improve the coverage. You can see which lines need tests in Codecov. Basically, you want to add some tests to |
|
@pipliggins the integration failing test ( |
…ged to logic operation
|
Check out this pull request on See visual diffs & provide feedback on Jupyter Notebooks. Powered by ReviewNB |
|
This PR includes the changes in #4470 so that one needs merging before this one can be reviewed. |
brosaplanella
left a comment
There was a problem hiding this comment.
Looks good to me, but would be good to have an extra pair of eyes on it. Not marking it as approved so it doesn't get accidentally merged.
DrSOKane
left a comment
There was a problem hiding this comment.
This looks excellent! I noticed you coded the equation for L_mig to avoid division by zero, while still ensuring the correct behaviour at zero current by making it a very large number. Well done!
kratman
left a comment
There was a problem hiding this comment.
Some cleanup is needed for this
…yBaMM into i3769_Kolzenberg2020
Co-authored-by: Eric G. Kratz <kratman@users.noreply.github.com>
Co-authored-by: Eric G. Kratz <kratman@users.noreply.github.com>
…yBaMM into i3769_Kolzenberg2020
kratman
left a comment
There was a problem hiding this comment.
From the infra/conflicts side this looks good to me
|
I will squash this after tests pass. @all-contributors please add @kawaMANMI for code |
|
@kawaMANMI already contributed before to code |
Description
Please include a summary of the change and which issue is fixed. Please also include relevant motivation and context. List any dependencies that are required for this change.
Another option for the SEI gowth has been added based on teh model of von Kolzenberg et al.
spefically equation (19) has been implemented. Tunilening distance and lithoum ion conductivity in teh SEI as parameters need to be defined here is example to run a case
Fixes #3769
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: