-
Notifications
You must be signed in to change notification settings - Fork 31
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
Add tests and fix for spherical spreading centers #520
Add tests and fix for spherical spreading centers #520
Conversation
|
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 good to me. Can you also add the fix for the mass conserving temperature model we discussed and update the test results?
const Point<3> point_1_cart = Utilities::spherical_to_cartesian_coordinates(point_1.get_array()); | ||
const Point<3> point_2_cart = Utilities::spherical_to_cartesian_coordinates(point_2.get_array()); | ||
|
||
return radius * std::acos(std::min(1.,std::max(0.,point_1_cart*point_2_cart/(radius*radius)))); |
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 nice! It would also open up the possibility in the future to allow Cartesian points to be passed to the function directly. One of the unit tests fail with this change, but I checked it and it actually gave an incorrect solution before (distance (-90,0)->(90,180) = pi, not zero). So you can update this test result.
Codecov ReportAttention:
Additional details and impacted files@@ Coverage Diff @@
## main #520 +/- ##
=======================================
Coverage 93.45% 93.46%
=======================================
Files 92 92
Lines 6279 6301 +22
=======================================
+ Hits 5868 5889 +21
- Misses 411 412 +1
Continue to review 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.
almost there, I think you forgot to add the .wb and .dat files for the test you made
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.
Great, thanks for your contribution!
Addresses Issue #518