MAINT: Use scipy builtins rotation to compute quaternions.#6241
MAINT: Use scipy builtins rotation to compute quaternions.#6241brisvag merged 5 commits intonapari:mainfrom
Conversation
As often, it is better to rely on existing battle tested code than roll our own. This also help testing the upstream code more. This replace our own quaternion to euler angles with using the Rotations call in scipy. This file comes from far at the beginning of napari, and had one significant modification in napari#2530 (WRT degenerate case), which I believe scipy already handle with the gimbals lock warning and is tested. Then α, β, γ is not unique when β=±π/2, then current new implementation would have `α=<some value>`, `γ=0`, the old code `α=0`, `γ=2.π-<some value>`. Note that the outputs are slightly different, with sometime the angles being +/- p , +/- zero i in some case. You can test this branch with this test script that has a few hardcoded degenerate case, as well as pick ~10k random quaternion and compare the resutls. https://gist.github.com/Carreau/53dfaefa0c07aa3a7a0bd90e890810bb ``` +/- zero [-2.86, -3.12, -2.88, -2.64] old 1.491, 0.000, 1.658 new 1.491, -0.000, 1.658 diff 0.000, 0.000, -0.000 ``` ``` +/- pi [1.44, 0.48, 3.93, -1.31] old -3.142, 0.702, -2.498 new 3.142, 0.702, -2.498 diff 6.283, 0.000, 0.000 ``` ``` gimbal lock: [0.09, -0.78, 0.09, 0.78] old 0.000, 1.571, -2.912 new 2.912, 1.571, 0.000 diff 2.912, 0.000, -2.912 [-0.64, -0.58, -0.64, 0.58] old 0.000, 1.571, 1.473 new 4.811, 1.571, 0.000 diff 4.811, 0.000, 1.473 ```
|
Opening as draft, if we think we are ok with this I can remove completely the old function and rework the documentation, but I wanted to make sure I don't miss anything. |
Codecov Report
@@ Coverage Diff @@
## main #6241 +/- ##
==========================================
+ Coverage 91.60% 91.65% +0.05%
==========================================
Files 583 584 +1
Lines 51364 51391 +27
==========================================
+ Hits 47051 47103 +52
+ Misses 4313 4288 -25
|
|
Added a commit that remove old impl and cleanup docs. |
|
as a note, this function is always used with |
brisvag
left a comment
There was a problem hiding this comment.
Nice! I'm in favour of removing the kwarg since it's unneeded.
| theta_1 = np.arctan2( | ||
| 2 * (q.w * q.z + q.y * q.x), | ||
| 1 - 2 * (q.y * q.y + q.z * q.z), | ||
| with warnings.catch_warnings(): |
There was a problem hiding this comment.
Each time the warnings.catch_warnings() is used then a list of already seen warnings is reset.
So using it in the context of often-used functions may result in bigger spamming of user. Why warning happen and how we avoid it?
There was a problem hiding this comment.
We can't avoid it because math, it is in SciPy and is triggered when the set of Euler Angles are degenerate and you are looking directly up a pole.
Can you point me to the issue or an explanation of "catch warnings" reseting the warning list ?
There was a problem hiding this comment.
I could send a pr to SciPy with a parameter gimbal_lock_ok:bool to prevent the warning.
Co-authored-by: Grzegorz Bokota <bokota+github@gmail.com>
|
@Carreau @brisvag personally I would keep Other than that I am +1 on these changes, minus the caveat from @Czaki — which I suppose would take some time, plus we try to follow SPEC0 for SciPy so it'll be like 2y? How do we handle this? Shall we fix that CPython issue? Wait that'll take longer... 😂 |
That's fair.
There is already a 5 Years Old PR that received no comments, so unless we can poke at some CPython core dev to get that fixed... Another possibility is to insert the filter once and for all. |
Would you be ok or prefer to rename to |
I do not feel that it is important to not emit warnings on non latest Scipy version. |
|
@Carreau yes I'm ok with renaming the function to include "degrees". 🙏 |
|
For reference: scipy/scipy#19306 |
…apari#6241)" This reverts commit be16a7d.
|
Do you want to reset hard main to scipy/spatial/transform/tests/test_rotation.py , and I can re-submit cc8d3ec ? This way we don't have a revert on Main ? |
|
As mentioned on zulip, no problem with reverting with a new PR, but more pain comes from rewriting history. Let's move on with a new Pr! |
No worries, I'll reopen a PR once I have some confirmation that SciPy has the featuer upstream |
This is a partial reapply of napari#6241, that was reverted, but reapply only the part where we simplify the API that has an unsed argument and rename the function. This is thus a partial revert of napari#6299 which itself is a revert of napari#6241
This is a partial reapply of napari#6241, that was reverted, but reapply only the part where we simplify the API that has an unsed argument and rename the function. This is thus a partial revert of napari#6299 which itself is a revert of napari#6241
This is a partial reapply of napari#6241, that was reverted, but reapply only the part where we simplify the API that has an unsed argument and rename the function. This is thus a partial revert of napari#6299 which itself is a revert of napari#6241
This is a partial reapply of napari#6241, that was reverted, but reapply only the part where we simplify the API that has an unsed argument and rename the function. This is thus a partial revert of napari#6299 which itself is a revert of napari#6241
This is a partial reapply of napari#6241, that was reverted, but reapply only the part where we simplify the API that has an unsed argument and rename the function. This is thus a partial revert of napari#6299 which itself is a revert of napari#6241
This is a partial reapply of napari#6241, that was reverted, but reapply only the part where we simplify the API that has an unsed argument and rename the function. This is thus a partial revert of napari#6299 which itself is a revert of napari#6241
As often, it is better to rely on existing battle tested code than roll our own. This also help testing the upstream code more.
This replace our own quaternion to euler angles with using the Rotations call in scipy.
This file comes from far at the beginning of napari, and had one significant modification in #2530 (WRT degenerate case), which I believe scipy already handle with the gimbals lock warning and is tested.
Then α, β, γ is not unique when β=±π/2, then current new implementation would have
α=<some value>,γ=0, the old codeα=0,γ=2.π-<some value>.Note that the outputs are slightly different, with sometime the angles being +/- pi , +/- zero i in some case.
You can test the first commit of this branch with this test script that has a few hardcoded degenerate case, as well as pick ~10k random quaternion and compare the resutls.
https://gist.github.com/Carreau/53dfaefa0c07aa3a7a0bd90e890810bb