Fix sign error in quaternion-to-Euler conversion at Phi = pi - #674
Fix sign error in quaternion-to-Euler conversion at Phi = pi#674gaoflow wants to merge 2 commits into
Conversation
The Phi = pi branch of qu2eu used a = -2 * qu[1] * qu[2], which negated the azimuth of the reconstructed axis. Rotation.to_euler() therefore returned wrong Bunge Euler angles for every 180 degree rotation about an axis in the xy-plane not aligned with x or y, and from_euler(to_euler(r)) recovered a reflected rotation rather than r. The correct term is a = 2 * qu[1] * qu[2], consistent with eu2qu at beta = pi and Rowenhorst et al. (2015) Eq. A.14. Add a round-trip regression test for equatorial two-fold rotations, and update the reduced-fundamental sampling test whose phi1 expectation had absorbed the wrong sign.
|
Hello, thanks for contributing to ORIX!. Good catch, and thank you for referencing the equations as well, it made this much easier to verify. For any other maintainers looking at this issue, another simple proof: These should be less than 1/10th of a degree apart, not almost 180. I'll add some minor edits this afternoon, then this will be ready for merging. Additionally, you should add your name to the init file and your name + affiliation to the zenodo.json file. I'll add my best guess as a suggestion later if you don't get to it first. You can also omit it if you choose, but if you contribute to ORIX, you should be credited on the DOI. |
|
Thanks — and nice extra proof, that one's cleaner than mine. Added myself to No rush on the edits, ping me if you'd rather I fold them in. |
argerlt
left a comment
There was a problem hiding this comment.
Approved. I was going to make some minor language changes originally (technically Quaternion.to_euler was fixed, not Rotation.to_euler), but I like your wording more, it's more relevant to the average user.
Waiting for #660 to be finished, then we will merge this to develop
Description of the change
Rotation.to_euler()returns wrong Bunge Euler angles for 180° rotations about an axis in the xy-plane that is not aligned with x or y (off-axis equatorial two-folds).from_euler(to_euler(r))then gives a different rotation (the axis is reflected).The cause is a sign error in the
Φ = πsingular branch ofqu2eu_single(orix/quaternion/_conversions.py), which handlesΦ = π ⇔ 180°rotations withw = z = 0:Inverting
eu2quatβ = πgivesqu[1] = -cos(δ),qu[2] = -sin(δ)withδ = (α - γ)/2, soα = arctan2(2·qu[1]·qu[2], qu[1]² - qu[2]²), i.e.a = +2·qu[1]·qu[2]. Both quaternion components carry the minus sign here, so their product is positive — unlike the neighbouringΦ = 0branch, where onlyqu[3]is negated anda = -2·qu[0]·qu[3]is correct. This matches Rowenhorst et al. (2015) Eq. A.14. The bug is masked whenxoryis zero (2·qu[1]·qu[2] = 0), which is the only equatorial case the existing tests covered.I audited the other singular branches of the conversion suite (
ax2qu/qu2axatω = 0, π,om2qufor 180° rotations,ho2ax, homochoric round-trips, and theΦ = 0branch ofqu2eu) via round-trip consistency and found them clean — this is a single localized sign.Minimal example of the bug fix
Notes
test_conversions.py(jit, pure-Python, and public-API paths).test_get_sample_reduced_fundamentalassertedunique(phi1) == [0, π/2]for C4/C6. That held only because the wrong sign collapsed the affected two-fold'sphi1ontoπ/2; ondevelopthose sampled rotations do not actually round-trip (matrix error 2.0). With the fixphi1gains its correct degenerate value3π/2and the whole sample round-trips, so I updated that expectation.pytest orix/testssuite passes locally.Progress of the PR
CHANGELOG.rst