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
Generalize GridTools::rotate
for arbitrary rotation axes.
#12816
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.
Otherwise, looks OK to me.
Adjusted this PR to have a unified interface between namespaces |
|
/rebuild |
Adjusting the interface for the |
Squashed the commits. Ready for final review. |
@@ -69,7 +70,7 @@ transform_grid(Triangulation<2> &tria, const unsigned int transform) | |||
// second round: rotate | |||
// triangulation | |||
case 1: | |||
GridTools::rotate(3.14159265358 / 2, tria); |
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.
That was a funny one - only 12 digits and then with wrong rounding (the first omitted digit of pi
is 9, so even in case this we would have found 12 digits sufficient it should have been ...5359
) - thanks for fixing things up.
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 was checking which tests are affected by the change by grepping for the function name and thought I could fix this along the way. There might be more occurrences of these magic numbers in the testsuite. I can check for these as a follow-up.
Something funny happened while adding your suggestions. PR is back on track now! |
We have the
Physics::Transformation::Rotations
namespace which we can use the generalize theGridTools::rotate
functions. Currently, the rotation matrices inGridTools::rotate
are hard-coded.I deprecated the old interface, in which you can specify the (Cartesian) rotation axis with an integer. Further, I removed some comments at the end of
grid_tools.h
which fixes the indentation of#include
directives. This makes this PR looks way larger than it actually is.I adjusted all tests to use the new function with the new interface. These tests only rotated triangulation around Cartesian axes. I can provide another test with an oblique orientation axis if you want to.
Further, the
Physics::Transformation::Rotations::rotation_matrix_3d
takes the axis parameter first and then the angle, while theGridTools::rotate
function takes the angle first and then the axis. Shall we take the chance and unify the interface?