Skip to content

Implement sodium-ion DFN - #4451

Merged
kratman merged 25 commits into
developfrom
na-ion
Oct 7, 2024
Merged

Implement sodium-ion DFN#4451
kratman merged 25 commits into
developfrom
na-ion

Conversation

@brosaplanella

@brosaplanella brosaplanella commented Sep 18, 2024

Copy link
Copy Markdown
Member

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.

  • New feature (non-breaking change which adds functionality)
  • Optimization (back-end change that speeds up the code)
  • Bug fix (non-breaking change which fixes an issue)

Key checklist:

  • No style issues: $ 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)
  • All tests pass: $ python run-tests.py --all (or $ nox -s tests)
  • The documentation builds: $ 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:

  • Code is commented, particularly in hard-to-understand areas
  • Tests added that prove fix is effective or that feature works

@review-notebook-app

Copy link
Copy Markdown

Check out this pull request on  ReviewNB

See visual diffs & provide feedback on Jupyter Notebooks.


Powered by ReviewNB

@codecov

codecov Bot commented Sep 18, 2024

Copy link
Copy Markdown

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 99.42%. Comparing base (bc53572) to head (00c4412).
Report is 165 commits behind head on develop.

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.
📢 Have feedback on the report? Share it here.

@rtimms rtimms left a comment

Copy link
Copy Markdown
Contributor

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

Comment thread src/pybamm/models/full_battery_models/sodium_ion/basic_dfn.py
@brosaplanella

Copy link
Copy Markdown
Member Author

Doctests are failing but I don't know why. @agriyakhetarpal can you help with this?

Comment thread src/pybamm/parameters/parameter_sets.py
Comment thread src/pybamm/input/parameters/sodium_ion/Chayambuka2022.py Outdated
Comment thread src/pybamm/input/parameters/sodium_ion/Chayambuka2022.py Outdated
Comment thread src/pybamm/models/full_battery_models/sodium_ion/basic_dfn.py
DrSOKane
DrSOKane previously approved these changes Oct 2, 2024

@DrSOKane DrSOKane left a comment

Copy link
Copy Markdown
Contributor

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.

@DrSOKane

DrSOKane commented Oct 2, 2024

Copy link
Copy Markdown
Contributor

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.

@kratman kratman left a comment

Copy link
Copy Markdown
Contributor

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

@kratman

kratman commented Oct 7, 2024

Copy link
Copy Markdown
Contributor

Docs all failed, so I will check on that too

@kratman

kratman commented Oct 7, 2024

Copy link
Copy Markdown
Contributor

Docs failures are unreleated. I am looking into it, but will fix it in a different PR

@kratman
kratman merged commit a7d476e into develop Oct 7, 2024
@kratman
kratman deleted the na-ion branch October 7, 2024 19:24
pkalbhor pushed a commit to pkalbhor/PyBaMM that referenced this pull request Nov 15, 2024
* 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>
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.

5 participants