Remove rotation-related functions #111
Conversation
hyrodium
force-pushed
the
remove_rotation
branch
from
db9f849 to
821fac0
Compare
Codecov Report
@@ Coverage Diff @@
## main #111 +/- ##
==========================================
+ Coverage 98.44% 99.20% +0.76%
==========================================
Files 1 1
Lines 193 126 -67
==========================================
- Hits 190 125 -65
+ Misses 3 1 -2
📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more |
sethaxen
left a comment
sethaxen
left a comment
There was a problem hiding this comment.
I'll have more suggestions later, but here are a few.
docs/src/examples/rotations.md
Outdated
| Rotations with quaternions have the following properties: | ||
|
|
||
| * An additive inverse of a unit quaternion represents the same rotation. | ||
| * Unit quaternion is more efficient to represent rotation than rotation matrix. |
There was a problem hiding this comment.
| * Unit quaternion is more efficient to represent rotation than rotation matrix. | |
| * A unit quaternion is more efficient for representing a rotation than a rotation matrix. |
And what if we made it explicit that the efficiency is that it uses only 4 numbers with 1 constraint instead of 9 numbers with 6 constraints?
There was a problem hiding this comment.
I have updated the part with:
- A unit quaternion (4 real numbers) is more efficient for representing a rotation than a rotation matrix (9 real numbers).
- This results in higher computational performance in terms of time, memory usage, and accuracy.
docs/src/examples/rotations.md
Outdated
| * High computation performance, both in time and memory | ||
| * A conjugate of a unit quaternion represents the inverse rotation. | ||
| * The quaternion has unit length, so conjugate and multiplicative inverse is the same. | ||
| * The set of unit quaternion ``\left\{w + ix + jy + kz \in \mathbb{H} \ | \ x, y, z \in \mathbb{R} \right\} \simeq S^3`` is isomorphic to ``SU(2)``. |
There was a problem hiding this comment.
I've seen this explained differently in a few places. But I would probably steer away from calling the set of unit quaternions S^3. It's isomorphic to it, but for one thing, we're not working with unit vectors. Strictly speaking, I'd say it's U(1, ℍ).
There was a problem hiding this comment.
Thank you for the suggestion! I was really looking for the short symbol for the set of unit quaternions, and
docs/src/examples/rotations.md
Outdated
| * A conjugate of a unit quaternion represents the inverse rotation. | ||
| * The quaternion has unit length, so conjugate and multiplicative inverse is the same. | ||
| * The set of unit quaternion ``\left\{w + ix + jy + kz \in \mathbb{H} \ | \ x, y, z \in \mathbb{R} \right\} \simeq S^3`` is isomorphic to ``SU(2)``. | ||
| * These groups are homomorphic to ``SO(3)``. |
There was a problem hiding this comment.
We probably need to define SO(3). Can this and the below statement be merged?
docs/src/examples/rotations.md
Outdated
|
|
||
| ## Rotate a vector with a quaternion | ||
| A vector ``v = (v_x, v_y, v_z)`` can be rotated by a unit quaternion ``q``. | ||
| The rotated vector ``v' = (v_x', v_y', v_z')`` can be obtained as |
There was a problem hiding this comment.
What if we said R(q)v instead of v' to avoid confusion with adjoint?
There was a problem hiding this comment.
The group of unit quaternions R(q)v notation.
I have replaced (v, v′) with (u,v).
| return pnew | ||
| end | ||
| function rotmatrix_from_quat(q::Quaternion) |
There was a problem hiding this comment.
Minor point, but I prefer we avoid _from_quat unless necessary, since in Julia, _from_XXX is handled by the type constraint in the method overload, so it's redundant.
There was a problem hiding this comment.
I agree with that as the common manner in the Julia language. However this is just documentation and readers cannot check the methods of the function by running methods(XXX), and I thought explicit XXX_from_YYY is a little better.
Co-authored-by: Seth Axen <[email protected]>
Co-authored-by: Seth Axen <[email protected]>
|
@sethaxen Thank you for the corrections, and sorry for my poor English:sweat_smile:. I think we can improve the documentation after the release of v0.7.0 if there are no major problems in this PR. |
You're right, no problem at all! LGTM. |
2 participants
This PR removes rotation-related functions. (closes #86)
Note that the documentation is failing to build because Rotations.jl assumesQuaternionhas thenormfield. I'd like to wait for JuliaGeometry/Rotations.jl#243.Edit: The documentation does not depend on Rotations.jl now.