-
-
Notifications
You must be signed in to change notification settings - Fork 597
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
Enable cylindrical and platelet particle shape options #4827
base: develop
Are you sure you want to change the base?
Conversation
the negative and positive electrodes.
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## develop #4827 +/- ##
========================================
Coverage 98.70% 98.70%
========================================
Files 303 303
Lines 23328 23346 +18
========================================
+ Hits 23025 23043 +18
Misses 303 303 ☔ 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.
In terms of the coding, this is flawless.
In terms of how useful this upgrade will be, I am not sure. While supervising a summer project with @katiezzzzz, I discovered that GITT measurements of diffusivity presuppose the particle to have a particular shape (usually spherical), which limits the utility of this feature. @brosaplanella I believe you looked into this but not sure if you ever published your findings.
Despite this, I would like to thank @d-cogswell for the code and strongly recommend its inclusion in PyBaMM. The question around diffusivity could be the subject of future work, by yourself of another PyBaMM user who can now hit the ground running.
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'm not sure this would work as is since standard_spatial_vars.r_n
will still be used in other places with the spherical coordinates (e.g. in the geometry, and in r_average
). Have you tested that this works?
See #4857 |
After some more testing, I'm noticing my solution here does not produce the same results as when the global variable is modified. This must be because the global variable is still being used in other places. |
I'm working on a modification that uses factory functions for |
I think it would be easier to just do #4857 |
Description
Enables
cylindrical
andplatelet
as model options forparticle shape
. A 2-tuple can also now be provided to assign different particle shapes at the anode and cathode. Cylindrical particles are discretized in cylindrical polar coordinates and given an area/volume ratio of 2/R. Platelet particles are discretized in cartesian coordinates and given an area/volume ratio of 1/R.The coordinate change is applied to copies of
r_n
andr_p
to avoid the bad programming practice of modifying global variables inpybamm.standard_spatial_vars
. This requires a minimal number of code changes while also insuring that changes made during the creation of a model are not accidentally inherited by subsequent models.The model options documentation for
particle shape
has been updated.Fixes #1072
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 -m pytest
(or$ nox -s tests
)$ python -m pytest --doctest-plus src
(or$ nox -s doctests
)You can run integration tests, unit tests, and doctests together at once, using
$ nox -s quick
.Further checks: