-
Notifications
You must be signed in to change notification settings - Fork 24
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 and spherical versions of SurfaceFlux #689
Cylindrical and spherical versions of SurfaceFlux #689
Conversation
@jhdark let me know what you think and I'll complete with documentation |
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #689 +/- ##
==========================================
+ Coverage 99.47% 99.48% +0.01%
==========================================
Files 58 58
Lines 2290 2349 +59
==========================================
+ Hits 2278 2337 +59
Misses 12 12 ☔ View full report in Codecov by Sentry. |
… SurfaceFluxSpherical classes
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.
This looks great to me, also like the fact that the units of the results are given in the doc strings, think this is smth we could implement in other export classes (accounting for differences in different dimensions).
However, just some small coverage tests that need to be added.
Then its good to go!
Maybe what we should do is add the units in the docstrings and also in the title of the csv column based on the dimension of the mesh |
I like the idea, although this could be a big change, we can add in another PR. I can make a new issue |
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.
LGTM
Proposed changes
This PR implements the SurfaceFlux in cylindrical and spherical coordinates:
SurfaceFluxCylindrical
andSurfaceFluxSpherical
as well as a little bit of refactoring.Usage:
Similarly for spherical meshes:
Types of changes
What types of changes does your code introduce to FESTIM?
Checklist