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.46% 99.46%
=========================================
Files 293 297 +4
Lines 22381 22512 +131
=========================================
+ Hits 22261 22392 +131
Misses 120 120 ☔ View full report in Codecov by Sentry. |
There was a problem hiding this comment.
thanks @brosaplanella looks good, can you add a test in the basic model tests? sorry, realise this is draft and not finished
|
|
||
| class BasicDFN(pybamm.lithium_ion.BaseModel): | ||
| """Doyle-Fuller-Newman (DFN) model of a sodium-ion battery, from | ||
| :footcite:t:`Marquis2019`. |
There was a problem hiding this comment.
Does the Na-ion paper have the same model equations? It would be good to reference that paper here for more context I think, and then say eqns are from Marquis (except they aren't quite ahah)
There was a problem hiding this comment.
Not quite, but because I took the parameters from the COMSOL implementation it is actually quite similar to the Marquis et al. Happy to cite either, not sure what will be less confusing to the users.
|
Doctests are failing but I don't know why. @agriyakhetarpal can you help with this? |
There was a problem hiding this comment.
@brosaplanella, I can't comment on the line directly since it hasn't been modified, so I'm commenting on the file – the fix is in line 29. Changing it to
:footcite:t:`Rieger2016`should fix the doctest failure.
brosaplanella
requested review from
martinjrobins,
Saransh-cpp,
kratman,
arjxn-py,
a team,
valentinsulzer,
TomTranter,
MarcBerliner and
DrSOKane
as code owners
| "Positive particle diffusivity [m2.s-1]": NVPF_diffusivity_Chayambuka2022, | ||
| "Positive electrode OCP [V]": NVPF_ocp_Chayambuka2022, | ||
| "Positive electrode porosity": 0.23, | ||
| "Positive electrode active material volume fraction": 1 - 0.23 - 0.22, |
There was a problem hiding this comment.
Any reason why this is not just a single number?
There was a problem hiding this comment.
This number is not directly reported in the paper, they provide the other two and say everything adds up to one. Given that one needs to go through the paper to find this, I thought it would be better to leave it this way so it is apparent what's going on. I will add a comment to make it clear.
There was a problem hiding this comment.
Personally, I think this just looks confusing. I don't see any harm in making it a single number
| "Negative particle diffusivity [m2.s-1]": HC_diffusivity_Chayambuka2022, | ||
| "Negative electrode OCP [V]": HC_ocp_Chayambuka2022, | ||
| "Negative electrode porosity": 0.51, | ||
| "Negative electrode active material volume fraction": 1 - 0.51 - 0.001, |
| # `param` is a class containing all the relevant parameters and functions for | ||
| # this model. These are purely symbolic at this stage, and will be set by the | ||
| # `ParameterValues` class when the model is processed. | ||
| param = self.param |
There was a problem hiding this comment.
if these are the same, then why not just use self.param instead?
There was a problem hiding this comment.
Actually, I just copied this from the lithium_ion BasicDFN. I believe we do that in all the basic models, so we don't have to do self.param every time (there any a lot of occurrences) and the code is easier to read. I personally think it is useful as it is easier to read through the model.
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: