ENH: Add ability to silence Gimbal Lock warnings for rotations. #19338
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
The test were slightly incorrect in the sens that they were testing that at least one of the combination of angle/sequence/intrinsic-extrinsic angle where remitting a Gimbal Lock warning, instead of each of them. The warning is now checked _only_ and on _all_ on the line that are supposed to emit such a warning. And while I was at it I parametrized the tests, so each appear as a separate inter when running with pytest, and extended to all the parametrized test in case it ever have an issue somewhere. In all of the `angles = np.array(...)` I have the impression that each one should emit a warning and that angle should likely also be a parameter, but it was starting to be a lot (maybe open an issue to parametrize angle?)
While it is possible to silence warnings with `catch_warnings()` context manager and a filter the approach has a few drawbacks: 1) The warnings are still emitted, which can have a perf impact 2) Using catch_warnings hits this bug `python/cpython#73858` that may reset a bunch of other warnings. I also slightly refactor the code by moving the warning 1 level up into `_compute_euler` in the stack and having the core function not emit the warnings, but return a tuple of (data, was_there_a_gimbal_lock). This in particular also emit the warnings only once per function call instead of once per gimbal lock. Another possibility would be to move the warning one more level up into `as_euler(...)` which is the only public method that uses `_compute_euler(...)`. The other one being `_as_euler_from_matrix(...)`, whcih is private and only use in tests. If I can to that, then I think it might be ok to not add the `gimbal_lock_ok` keyword and just tell users to reach for the private `_compute_euler`, and discard the warning flag. We could also return a boolean array of whcich rotations are Gimbal Locked. I don't know if that would be useful. Closes scipy#19306
Carreau
added a commit
to Carreau/scipy
that referenced
this pull request
Nov 12, 2023
While it is possible to silence warnings with `catch_warnings()` context manager and a filter the approach has a few drawbacks: 1) The warnings are still emitted, which can have a perf impact 2) Using catch_warnings hits this bug `python/cpython#73858` that may reset a bunch of other warnings. Thus we change the core of the cython functions to return an extra boolean when a gimbals lock was detected and emit the warnings only once. This does mean that the gimbals warning is emitted only once when computed on an array. I guess this might have a perf impact as we emit the warning only once. We could also refactor to store an int and say how many gimbal lock were detected. (redo of scipy#19338)
|
closing in favor of #19512, it was a bit too complicated to rebase, and i'll keep this for history sake. |
Carreau
added a commit
to Carreau/scipy
that referenced
this pull request
Nov 14, 2023
While it is possible to silence warnings with `catch_warnings()` context manager and a filter the approach has a few drawbacks: 1) The warnings are still emitted, which can have a perf impact 2) Using catch_warnings hits this bug `python/cpython#73858` that may reset a bunch of other warnings. Thus we change the core of the cython functions to return an extra boolean when a gimbals lock was detected and emit the warnings only once. This does mean that the gimbals warning is emitted only once when computed on an array. I guess this might have a perf impact as we emit the warning only once. We could also refactor to store an int and say how many gimbal lock were detected. (redo of scipy#19338)
Carreau
added a commit
to Carreau/scipy
that referenced
this pull request
Nov 20, 2023
While it is possible to silence warnings with `catch_warnings()` context manager and a filter the approach has a few drawbacks: 1) The warnings are still emitted, which can have a perf impact 2) Using catch_warnings hits this bug `python/cpython#73858` that may reset a bunch of other warnings. Thus we change the core of the cython functions to return an extra boolean when a gimbals lock was detected and emit the warnings only once. This does mean that the gimbals warning is emitted only once when computed on an array. I guess this might have a perf impact as we emit the warning only once. We could also refactor to store an int and say how many gimbal lock were detected. (redo of scipy#19338) some review Update scipy/spatial/transform/_rotation.pyx
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This is on top of #19302 which introduce only a test refactor to make this one much easier to read. You may want to look at the last commit on its own when reviewing the tests.
Therefore opening only as draft for now.
While it is possible to silence warnings with
catch_warnings()context manager and a filter the approach has a few drawbacks:python/cpython#73858that may reset a bunch of other warnings.I also slightly refactor the code by moving the warning 1 level up into
_compute_eulerin the stack and having the core function not emit the warnings, but return a tuple of (data, was_there_a_gimbal_lock). This in particular also emit the warnings only once per function call instead of once per gimbal lock.Another possibility would be to move the warning one more level up into
as_euler(...)which is the only public method that uses_compute_euler(...). The other one being_as_euler_from_matrix(...), whcih is private and only use in tests. If I can to that, then I think it might be ok to not add thegimbal_lock_okkeyword and just tell users to reach for the private_compute_euler, and discard the warning flag.We could also return a boolean array of whcich rotations are Gimbal Locked. I don't know if that would be useful.
Closes #19306