-
-
Notifications
You must be signed in to change notification settings - Fork 563
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
Cylindrical geometry operators #1824
Conversation
Codecov Report
@@ Coverage Diff @@
## develop #1824 +/- ##
========================================
Coverage 99.26% 99.26%
========================================
Files 345 345
Lines 18954 18979 +25
========================================
+ Hits 18815 18840 +25
Misses 139 139
Continue to review full report at Codecov.
|
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, looks good! Lots of conflicts coming up with #1810 :(
@@ -30,6 +30,9 @@ | |||
|
|||
y = pybamm.SpatialVariable("y", domain="current collector", coord_sys="cartesian") | |||
z = pybamm.SpatialVariable("z", domain="current collector", coord_sys="cartesian") | |||
r = pybamm.SpatialVariable( | |||
"r", domain="current collector", coord_sys="cylindrical polar" | |||
) |
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 wonder if we should be more specific with this one, like r_macro
maybe?
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.
yeah, I agree
elif submesh.coord_sys == "cylindrical polar": | ||
second_dim_repeats = self._get_auxiliary_domain_repeats(symbol.domains) | ||
|
||
# create np.array of repeated submesh.edges | ||
r_edges_numpy = np.kron(np.ones(second_dim_repeats), submesh.edges) | ||
r_edges = pybamm.Vector(r_edges_numpy) |
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.
reduce code duplication here
2 * np.pi * (1 - r0), | ||
decimal=3, | ||
) | ||
|
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.
we really need to clean up this test file 😅
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.
Haha tell me about it. My favourite bit was the test that said it was testing div(grad(r^2))=6 but it was actually testing div(grad(6))=0...
There's no rush to merge this, so I'll wait, fix conflicts with #1810 and also have a go at tidying up these tests a bit.
Description
Adds cylindrical finite volume method.
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:
$ flake8
$ python run-tests.py --unit
$ cd docs
and then$ make clean; make html
You can run all three at once, using
$ python run-tests.py --quick
.Further checks: