Repository navigation
Make mplot3d mouse rotation style adjustable #28841
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Merged
scottshambaugh
merged 14 commits into
matplotlib:main
from
MischaMegens2:mplot3d-mouse-rotation-style
Oct 15, 2024
Merged
Changes from 1 commit
Commits
Show all changes
14 commits
Select commit
Hold shift + click to select a range
077ba10
Make mplot3d mouse rotation style adjustable
MischaMegens2 133c916
Implement review suggestions
MischaMegens2 6ffebd8
Update view_angles.rst: polar plot -> spherical coordinate plot
MischaMegens2 d35e0cf
Update lib/mpl_toolkits/mplot3d/tests/test_axes3d.py
MischaMegens2 9421ad0
Update doc/api/toolkits/mplot3d/view_angles.rst
MischaMegens2 e7665b7
Update doc/users/next_whats_new/mouse_rotation.rst
MischaMegens2 686f0ca
Suggestions from the reviewer
MischaMegens2 a293e31
Replace Holroyd by Bell
MischaMegens2 89701eb
Soften the arcball edge using a 'border' parameter
MischaMegens2 6b15663
np.clip() to avoid floating point round-off errors
MischaMegens2 eaa51a4
Add np.clip() to avoid floating point round-off errors
MischaMegens2 b62ee99
Fix documentation: view_angles.rst, change default style
MischaMegens2 c71ab58
Revert to arcball style
MischaMegens2 bcffb92
Fix doc build error in view_angles.rst; add description of border
MischaMegens2 File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
np.clip() to avoid floating point round-off errors
Add np.clip() to avoid floating point round-off errors on the macos github CI runners, see also #28823 (comment)
- Loading branch information
commit 6b156630a789afa31015701966c770b29fbe1841
There are no files selected for viewing
This file contains hidden or 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
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It won't let me add this comment to the unmodified line below, but in playing with this PR I do infrequently run into a domain error on arcsin due to floating point errors. A little hard to reproduce reliably, but it does pop up. Clipping the inside value to [-1, 1] should fix this.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm just so curious about the values of qw, qx, qy, and qz that cause this. I can understand the
posx and posy should be finite valuescomplaint, as a consequence of the arcsin argument getting out of range. But theinvalid value encountered in arcsinis still mysterious...Could you try for me sometime replacing the offending statement with the following:
(I thought the error merits an attempt at diagnosing; not that I'm opposed to the np.clip(), I just thought it would be good to understand what is going on, how this comes about.)
Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I just spent a few minutes trying to reproduce but couldn't - it's a tricky edge case to trigger. When I ran across it before, the value inside arcsin was only 1e-16 (or thereabouts) larger than 1, so it's just a result of numerical round-off. My guess would be a conditioning issue, where one of the values is very small relative to the others and gets rounded off to 0 in the denominator, but is big enough to still impact the numerator. I'll leave the debug lines in and let you know if it happens again, but am not concerned - this stuff end up happening fairly regularly across the codebase.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think the np.clip got missed in your latest commit, possible to add that quickly before merge?
Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh sorry, of course; I put the np.clip() back in.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks! Need to move the close paren to fix the test failure but the MR is looking good.
Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You had mentioned before that there were floating point round-off errors on the macos github CI runners (#28823 (comment)); I think that was the original motivation to put the np.clip() in in the first place. Any chance we can trigger the edge case there once again?
(Not that I'm against the np.clip(), but I'm still surprised by the occurrence of the quotient >1. I thought the IEEE 754 standard for floating-point arithmetic requires that multiplication and addition should be correctly rounded, so I thought we would be in the clear... unless macos would not conform to IEEE 754, which would be also quite remarkable... Or I don't quite understand all of the correctly, and I should go read some more Kahan)
Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If I happen to catch it again I'm happy to add it as a test! I think I lost the specific test case in the other MR that was erroring, and it was dependent on the specific math operations being performed so it likely wouldn't translate to this new code.
To be clear, I don't think it's a macos-specific problem, the issue is more that there isn't perfect determinism across different platforms / python versions / etc (even with perfect determinism within a configuration). Everything is being rounded "correctly", it's just that if we're right on the edge of floating point tolerance, complier/processor/implementation/optimization differences can have different results, and values infinitesimally on the wrong side of 1 will stack up unfavorably with further operations. For a simple concrete example, some systems in calculating
(a + b)*cmight distribute that multiplication toa*c + b*c, and those can result in different rounding of the results. Especially if for examplea >> band sobdoesn't make it into the mantissa ofa + b(addition and subtraction are not associative in floating point math, which is pretty unintuitive IMO).There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm just trying to wrap my head around what the optimization difference could have been that would lead to the observed result. '(a+b)c' -> 'ac + b*c' does not quite fit the bill... My hope is that an actual example would shed light on it...