ENH: Adding Modified Rodrigues Parameters to the Rotation class #12667
Conversation
tylerjereddy
left a comment
tylerjereddy
left a comment
There was a problem hiding this comment.
Just added a few minor comments.
@nmayorov will probably have more detailed comments
I see the manuscript has been cited > 2000 times, which may help for justification of breadth of interest, but I'm not immediately familiar with this myself.
scipy/spatial/transform/rotation.pyx
Outdated
| Notes | ||
| ----- | ||
|
|
||
| .. versionadded:: 1.5.3 |
There was a problem hiding this comment.
Just FYI -- this will probably have to be 1.6.0 if the feature is approved because we have already released the 1.5.x series and almost always restrict point releases to bug fixes rather than enhancements/new features.
scipy/spatial/transform/rotation.pyx
Outdated
| quat[ind, 0] = 2 * cmrp[ind, 0] / (1 + ss) | ||
| quat[ind, 1] = 2 * cmrp[ind, 1] / (1 + ss) | ||
| quat[ind, 2] = 2 * cmrp[ind, 2] / (1 + ss) | ||
| quat[ind, 3] = (1 - ss) / (1 + ss) |
There was a problem hiding this comment.
minor: not sure if the compiler will optimize the repeated (1 + ss) calls or if assigning the value to i.e., a variable once might help
There was a problem hiding this comment.
I'm not sure either, I went ahead and changed ss to 1+ss
scipy/spatial/transform/rotation.pyx
Outdated
| Notes | ||
| ----- | ||
|
|
||
| .. versionadded:: 1.5.3 |
I'm not sure myself what the breadth of interest is. I'd never heard of them until I started taking a course on Coursera for spacecraft attitude control. Perhaps it would be better to split this work out into it's own little Python library and then merge it into scipy if it seems popular enough? |
|
MRP fits well into Rotation, it's a small incremental addition which I support. I will review on the weekend. |
scipy/spatial/transform/rotation.pyx
Outdated
| def from_mrp(cls, mrp): | ||
| """Initialize from Modified Rodrigues Parameters (MRPs). | ||
|
|
||
| MRPs are a 3 element attitude description commonly used in the aerospace |
There was a problem hiding this comment.
I think a more substantive definition is required here.
The easiest way it to define it through axis and angle of rotation, perhaps pointing out similarity to rotation vector but with different scaling.
There was a problem hiding this comment.
description -> representation?
Do you think it's valuable to mention its usage in the aerospace industry? I'd say all common representations are widely used in the aerospace industry. I would probably suggest removing.
There was a problem hiding this comment.
I went ahead and used your provided definition. I think MRPs are not widely used outside of aerospace, so I guess I'd say the target audience for that factoid is people outside of the aero industry, but I don't think it hurts one way or another if the factoid is in the docs or not, we can leave it out.
scipy/spatial/transform/rotation.pyx
Outdated
|
|
||
| Initialize a single rotation: | ||
|
|
||
| >>> r = R.from_mrp([0, 0, 0.41421356]) |
There was a problem hiding this comment.
I suggest using unity MRP that corresponds to rotation of 180 degrees.
Perhaps use as_euler in degrees to show this correspondence.
scipy/spatial/transform/rotation.pyx
Outdated
| >>> r.as_rotvec().shape | ||
| (2, 3) | ||
|
|
||
| It is also possible to have a stack of a single rotaton: |
There was a problem hiding this comment.
I think this example is not necessary.
There was a problem hiding this comment.
Yes, but all the other definitions have this same example.
There was a problem hiding this comment.
The examples in Rotation are not very good, no point following them in my opinion.
scipy/spatial/transform/rotation.pyx
Outdated
| cdef double ss, qnorm | ||
|
|
||
| for ind in range(num_rotations): | ||
| ss = 1 + _dot3(cmrp[ind, :], cmrp[ind, :]) |
There was a problem hiding this comment.
Perhaps name it at least denom or denominator.
scipy/spatial/transform/rotation.pyx
Outdated
| def as_mrp(self): | ||
| """Represent as Modified Rodrigues Parameters (MRPs). | ||
|
|
||
| 3D rotations can be represented using Modified Rodrigues Parameters, which |
There was a problem hiding this comment.
Again work on definition and make sure they are the same in from_mrp and as_mrp.
| >>> r.as_mrp().shape | ||
| (3,) | ||
|
|
||
| Represent a stack with a single rotation: |
There was a problem hiding this comment.
Again I suggest removing this marginal example.
scipy/spatial/transform/rotation.pyx
Outdated
| cdef double[:] quat | ||
|
|
||
| for ind in range(num_rotations): | ||
| if self._quat[ind, 3] < 0: # w > 0 to ensure 0 <= angle <= pi |
There was a problem hiding this comment.
I would probably not introduce quat variable just adjust the formula for mrps components depending on the case (using if in loop by i or before loop).
|
|
||
|
|
||
| def test_from_1d_single_mrp(): | ||
| mrp = [0, 0, 0.41421356] |
There was a problem hiding this comment.
Prefer using "exact" constants, like 0.25 * np.pi.
I guess unity MRP is a better testing example.
|
|
||
|
|
||
| def test_from_generic_mrp(): | ||
| mrp = [ |
There was a problem hiding this comment.
Ugly formatting (and more cases below).
| [0, 0, 0, 1] | ||
| ]) | ||
| assert_array_almost_equal( | ||
| Rotation.from_mrp(mrp).as_quat(), |
There was a problem hiding this comment.
Questionable formatting here as well.
There was a problem hiding this comment.
Resolved, along with others. Do you guy prefer to hit the "resolve conversation" button when a comment is addressed or just leave it?
|
I've updated the PR to address comments. In the update I have
|
scipy/spatial/transform/rotation.pyx
Outdated
| """Initialize from Modified Rodrigues Parameters (MRPs). | ||
|
|
||
| MRPs are a 3 element attitude description commonly used in the aerospace | ||
| industry. It is similar to the rotation vector, except instead of scaling |
There was a problem hiding this comment.
Maybe just following definition in from_rotvec is a good idea.
MRPs is a 3 dimensional vector co-directional to the axis of rotation and whose magnitude is equal to tan(theta / 4), where theta is the angle of rotation (in radians) [1].
scipy/spatial/transform/rotation.pyx
Outdated
| the rotation vector by the angle of rotation, is it scaled by tan(theta/4). | ||
|
|
||
| MRPs have a singuarity at 360 degrees which can be easily avoided by ensuring | ||
| that the angle of rotation is always <= 180 |
There was a problem hiding this comment.
is always <= 180 -> doesn't exceed 180 degrees?
scipy/spatial/transform/rotation.pyx
Outdated
| industry. It is similar to the rotation vector, except instead of scaling | ||
| the rotation vector by the angle of rotation, is it scaled by tan(theta/4). | ||
|
|
||
| MRPs have a singuarity at 360 degrees which can be easily avoided by ensuring |
scipy/spatial/transform/rotation.pyx
Outdated
| ------- | ||
| rotation : `Rotation` instance | ||
| Object containing the rotations represented by input rotation | ||
| vectors. |
scipy/spatial/transform/rotation.pyx
Outdated
| Initialize a single rotation: | ||
|
|
||
| >>> r = R.from_mrp([0, 0, 1]) | ||
| >>> r.as_euler('xyz') |
There was a problem hiding this comment.
Add degrees=True or use as_rotvec otherwise.
| >>> r.as_euler('xyz').shape | ||
| (2, 3) | ||
|
|
||
| It is also possible to have a stack of a single rotation: |
There was a problem hiding this comment.
I saw your consistency argument.
Not sure, I'm fine with either keeping or removing it.
scipy/spatial/transform/rotation.pyx
Outdated
| sign = -1 if self._quat[ind, 3] < 0 else 1 | ||
|
|
||
| for i in range(3): | ||
| mrps[ind, i] = sign * self._quat[ind, i] / (1 + sign * self._quat[ind, 3]) |
There was a problem hiding this comment.
Moving cdef double denominator = 1 + sign * self._quat[ind, 3] outside of the loop is a reasonable option.
| [1, 2, 2], | ||
| [1, -1, 0.5], | ||
| [0, 0, 0] | ||
| ]) |
There was a problem hiding this comment.
Super minor, but I'd put ]) on the previous line (in all places).
|
New update with latest feedback: |
scipy/spatial/transform/rotation.pyx
Outdated
| """Initialize from Modified Rodrigues Parameters (MRPs). | ||
|
|
||
| MRPs is a 3 dimensional vector co-directional to the axis of rotation and whose | ||
| magnitude is equal to tan(theta / 4), where theta is the angle of rotation (in radians) [1]. |
There was a problem hiding this comment.
I suggest adding double ticks (``) around "tan(theta / 4)" and "theta" to render it as code/expression.
The reference should be done as [1]_.
| # to a rotation of <= 180 | ||
| sign = -1 if self._quat[ind, 3] < 0 else 1 | ||
|
|
||
| denominator = 1 + sign * self._quat[ind, 3] |
There was a problem hiding this comment.
I suggest adding a test which will show that a returned MRPs will be correct and have magnitude <= 180 degrees, when the rotation was created with magnitude >= 180 degrees (through Euler or rotation vector). The scalar quaternion component must be negative in this case.
Reasoning - to test that this mathematical transformation does in fact work, because it's not obvious.
Added: but actually it might be considered obvious, because we just flip quaternion sign here. But still I think it won't hurt to test it.
There was a problem hiding this comment.
I think that's a good idea, test added
This is an attitude description commonly used in aerospace.
|
My latest changes seem to be causing some Windows failures, but I'm having trouble figuring out exactly what's going on. Any help? |
Not related to your changes, I should be able to fix shortly |
|
It looks very good now, I'm merging. Thank you for the contribution! |
Great working with you, looking forward to future contributions! |
|
Thanks @nbelakovski . Please have a moment to boast about this in the release notes ;) https://github.com/scipy/scipy/wiki/Release-note-entries-for-SciPy-1.6.0 |
4 participants
Reference issue
N/A
What does this implement/fix?
The implements 2 new methods in the Rotation class of the spatial/transform package, as_mrp and from_mrp, which allow the class to accept input and provide output in the form of Modified Rodrigues Parameters.