-
-
Notifications
You must be signed in to change notification settings - Fork 568
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
Implement sodium-ion DFN #4451
Implement sodium-ion DFN #4451
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 #4451 +/- ##
========================================
Coverage 99.42% 99.42%
========================================
Files 295 297 +2
Lines 22599 22685 +86
========================================
+ Hits 22468 22554 +86
Misses 131 131 ☔ View full report in Codecov by Sentry. |
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 @brosaplanella looks good, can you add a test in the basic model tests? sorry, realise this is draft and not finished
Doctests are failing but I don't know why. @agriyakhetarpal can you help with this? |
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.
Amazing work! The next step will be to implement the full DFN (with submodels) for sodium-ion, but this is a welcome and necessary first step.
All you need to do is edit CHANGELOG and this should be ready to merge. |
tests/integration/test_models/test_full_battery_models/test_sodium_ion/test_basic_models.py
Outdated
Show resolved
Hide resolved
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 can't vouch for the physics, but the code seems fine to me
Docs all failed, so I will check on that too |
Docs failures are unreleated. I am looking into it, but will fix it in a different PR |
* add sodium-ion model, parameters and examples * update citation keys to be consistent * rename electrolyte conductivity from kappa to sigma * add tests for sodium ion * fix doctests * fix doctest * remove accidentally added example * commented better the volume fraction * write volume fractions as a single number, not a substraction * improve basic tests * update CHANGELOG --------- Co-authored-by: Eric G. Kratz <kratman@users.noreply.github.com>
Description
Implemented DFN for sodium-ion (same as lithium-ion but in a separate file to avoid conflicts). Implemented parameter value from Chayambuka et al (2022) (actual values and data taken from this COMSOL example).
When adding the CITATION I also modified many citation keys to be consistent with the format (FirstAuthorYear). Hence the many files changed.
Fixes #4397 (not an issue but a discussion)
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: