Output bingham average as quaternions - #7289
Conversation
tjhei
left a comment
tjhei
left a comment
There was a problem hiding this comment.
We typically try to deprecate old parameter names or at least include a "conversion script". Not sure what the right choice here is...
| * called 'Output rotation as'. | ||
| */ | ||
| bool use_rotmat; | ||
| std::string output_rotation; |
There was a problem hiding this comment.
Should this be called "output format"?
| * follows from the quaternion algebra. | ||
| * (https://en.wikipedia.org/wiki/Quaternions_and_spatial_rotation) | ||
| */ | ||
| Tensor<2,3> quaternion_to_rotation_matrix(const double w,const double x, const double y, const double z); |
There was a problem hiding this comment.
Why are you using separate arguments instead of an array here?
There was a problem hiding this comment.
I oriented myself on zxz_euler_angles_to_rotation_matrix, but yes it would be more consistent to use one argument.
| * Quaternions are a double cover of 3x3 orthogonal matrices. R(q) = R(-q) | ||
| * and we choose the convention to only work with quaternions that fulfill q[0] > 0. | ||
| */ | ||
| std::array<double,4> rotation_matrix_to_quaternion(const Tensor<2,3> &rotation_matrix, const long double tolerance); |
There was a problem hiding this comment.
Do you need to document the order of the 4 return values?
|
Okay i now reintroduced the old parameter "Use rotation matrix" and changed its description. |
|
@tjhei this is ready for another review |
|
also @Wang-yijun @KiralyAgi i think you are the only other two using the |
2 participants
In this pull request i added the possibility to output rotations created by the bingham average particle property plugin as quaternions. Unit quaternions are a representation of rotations that is more numerically stable and compact than rotation matrices and avoid representation problems such as ambiguity and gimbal lock that euler angles struggle with.
The space of unit quaternions is a double cover of the space of rotations i.e. q and -q correspond to the same rotation matrix. Therefore the convention is used, where the scalar component of any quaternion is set to be >0.
The pull request contains:
In case I should add more documentation for the general concept of quaternions somewhere let me know!
Also in case removing a parameter and directly replacing it with a new one is bad style please please let me know and i will keep the old parameter as deprecated somehow.
Some comments:
I am planning to add a separate particle interpolator for rotations based on quaternions.
(preparations for this are in PR #7288).
The interpolator is already written, but i thought it would be nice to split the pull requests.
The new rotation average will contain a small benchmark and will be used in a more thorough benchmark (see PR #7231)
Stay tuned and thanks for anyone who is willing to review these additions.
Cheers!
Before your first pull request:
For all pull requests:
I used AI to understand and reconcile problems compiling my incorrect c++ code.
If yes, please describe your usage of AI models in the creation of this pull request
For new features/models or changes of existing features: