Skip to content
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

Correct roll angle units, issue #28256 #28261

Merged
merged 3 commits into from
May 24, 2024

Conversation

MischaMegens2
Copy link
Contributor

In mplot3d.Axes3D._on_move(), pass roll angle to view_init() in degrees (as it expects; not converted to radians, mistakenly).
Resolves #28256.

Pass roll angle to view_init() in degrees (not radians)
Copy link

@github-actions github-actions bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you for opening your first PR into Matplotlib!

If you have not heard from us in a week or so, please leave a new comment below and that should bring it to our attention. Most of our reviewers are volunteers and sometimes things fall through the cracks.

You can also join us on gitter for real-time discussion.

For details on testing, writing docs, and our review process, please see the developer guide

We strive to be a welcoming and open project. Please follow our Code of Conduct.

@scottshambaugh
Copy link
Contributor

Thanks for catching this! Is there a way that you can modify the existing mouse drag tests or add a new one to check for this? Those tests are a bit tricky to write / understand since they spoof mouse movements, so if there isn't an easy way to do that I think I would be fine with skipping it.

@scottshambaugh scottshambaugh added this to the v3.9.1 milestone May 20, 2024
@MischaMegens2
Copy link
Contributor Author

Thanks for catching this! Is there a way that you can modify the existing mouse drag tests or add a new one to check for this? Those tests are a bit tricky to write / understand since they spoof mouse movements, so if there isn't an easy way to do that I think I would be fine with skipping it.

I suppose I could try - but which existing tests did you have in mind? It seems most tests are for roll=0, with the exception of test_bar3d_shaded(), test_shared_view() and test_format_coord(), but these are clearly for other purposes than roll specifically.
We might add a variation on test_axes3d_rotated() with non-zero roll, for starters; and turn test_pan() into a test_rotate() for the mouse, since the rotation doesn't have much test coverage, apparently. And then add a test_rotate_with_roll(). Does that sound like a plan? (Or does it sound like it needs a separate PR? Please advise...)

@scottshambaugh
Copy link
Contributor

I was thinking about test_pan and test_toolbar_zoom_pan, but it looks like we don't actually have an equivalent test for rotating the view. If you want to take a shot at writing a similar test for rotating go for it, but it might be tricky so if you're not comfortable with that then we can merge as-is and I'll open an issue to add those tests.

https://github.com/matplotlib/matplotlib/blob/main/lib/mpl_toolkits/mplot3d/tests/test_axes3d.py#L1769-L1880

@MischaMegens2
Copy link
Contributor Author

I'll give it a try and let you know, it might take me a day or two

Add tests for rotation using the mouse (test_rotate), with and without roll. The test with nonzero roll fails if the roll angle is passed with wrong units (issue matplotlib#28256).
Copy link
Contributor

@scottshambaugh scottshambaugh left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks good to me, approved and once another maintainer adds their approval as well it will be merged in. The "codecov/project/tests" CI failure is more informational and not a blocker.

Thank you and congratulations on your first contribution to matplotlib! We hope to see more of you, and contributions are always welcome!

@MischaMegens2
Copy link
Contributor Author

[...] We hope to see more of you, and contributions are always welcome!

Well, now that we're at it...
Part of your commit 'Add ability to roll the camera in 3D plots' (#21426, 3 year ago...) reads:
- Make elev, azim, roll ordering consistent everywhere
This sounds like a good idea to me. However, I am puzzled by your choice of ordering (elev, azim): it is just the opposite of the matlab convention; and when I google:

  • "azimuth, elevation" -> 291 000 results
  • "elevation, azimuth"-> 41 500 results

And similarly:

  • "azimuth, elevation, roll" -> 1380
  • "elevation, azimuth, roll" -> 188

That is to say, the ordering you chose is not the common one. And if you consider that searching for both yields

  • "azimuth, elevation" and "elevation, azimuth" -> 37 500 results

(some links are non-committal), then it becomes clear that the elevation, azimuth order is really a minority position: (41-37)/(291-37) < 2%.
Moreover, the ordering with elevation first does not correspond to the order in which the 3 rotations take place.

Should we raise an issue to change it?
(You had the good foresight to use named parameters, so it is not a completely breaking change...)

@scottshambaugh
Copy link
Contributor

scottshambaugh commented May 24, 2024

I had a comment and it was mostly wrong when I actually went back to look, so ignore that email. I forget the reasoning behind changing the argument order, but I believe it’s to match the order in which the rotations are applied.

I’d be hesitant to change it again. Matching Matlab convention is nice, but not an actual goal of the matplotlib library. The kwargs are clear and unambiguous as-is, and we try to minimize API changes per this policy: https://matplotlib.org/devdocs/devel/api_changes.html

@MischaMegens2
Copy link
Contributor Author

MischaMegens2 commented May 24, 2024

I had a comment and it was mostly wrong when I actually went back to look, so ignore that email. I forget the reasoning behind changing the argument order, but I believe it’s to match the order in which the rotations are applied.

I’d be hesitant to change it again. Matching Matlab convention is nice, but not an actual goal of the matplotlib library. The kwargs are clear and unambiguous as-is, and we try to minimize API changes per this policy: https://matplotlib.org/devdocs/devel/api_changes.html

I understand the hesitation, and I don't particularly care about what Matlab's convention is either. But I do care that all the rest of the world appears to do something else than what matplotlib does at the moment. And the matplotlib order emphatically does not match the order in which the rotations are applied: that is azim, elev, roll.
So now the reasons to change it are 3 to 1 (doing things properly vs. eternal compatibility with the horrors of the past). There is a clear benefit to changing the order. Arguably it could be worth the effort of adapting existing code, as per https://matplotlib.org/devdocs/devel/api_changes.html (especially in view of the fact that the effort might be limited, with the named parameters). I thought it might make sense?

@oscargus oscargus merged commit 00c82ce into matplotlib:main May 24, 2024
41 of 42 checks passed
meeseeksmachine pushed a commit to meeseeksmachine/matplotlib that referenced this pull request May 24, 2024
@scottshambaugh
Copy link
Contributor

scottshambaugh commented May 24, 2024

Elevation and azimuth are applied at the same time, in effect neither comes before the other.

https://github.com/matplotlib/matplotlib/blob/main/lib/mpl_toolkits/mplot3d/axes3d.py#L1220-L1222

@scottshambaugh
Copy link
Contributor

scottshambaugh commented May 24, 2024

Thinking about it more, I think you’re more right about the rotation order if this is conceptualized as Euler angles. If thinking about extrinsic rotations, then the rotation order is roll, elev, then azim. If thinking about intrinsic rotations then the rotation order is azim, elev, then roll. The definitions in the docs are still unambiguous so I still think this is fairly marginal to make a change of API, but maybe @oscargus can weigh in.

@MischaMegens2 MischaMegens2 deleted the roll-angle-units branch May 25, 2024 03:05
timhoffm added a commit that referenced this pull request May 25, 2024
…261-on-v3.9.x

Backport PR #28261 on branch v3.9.x (Correct roll angle units, issue #28256)
@MischaMegens2
Copy link
Contributor Author

Thinking about it more, I think you’re more right about the rotation order if this is conceptualized as Euler angles.

Well actually, to be precise, mplot3d's azim, elev, roll are a kind of Tait-Bryan angles, not classic Euler angles (there exist 12 kinds of each, not counting the signs...)

If thinking about extrinsic rotations, then the rotation order is roll, elev, then azim. If thinking about intrinsic rotations then the rotation order is azim, elev, then roll.

Ah, yes, now we're talking!
The corresponding quaternion (for the intrinsic rotation) is
q = exp((+roll/2)*e_x)*exp((+elev/2)*e_y)*exp((-azim/2)*e_z)
i.e., Tait-Bryan angles, -z,y',x".

The definitions in the docs are still unambiguous so I still think this is fairly marginal to make a change of API, but maybe @oscargus can weigh in.

Ah yes. So perhaps it is time to open a separate Issue and close this PR ;)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Status: Waiting for author
3 participants