Repository navigation
[MNT]: mplot3d azim-elev-roll order #28353
Description
Summary
Part of PR #21426 'Add ability to roll the camera in 3D plots' (by Scott Shambaugh, 3 years ago) read as follows:
- Make elev, azim, roll ordering consistent everywhere
However, the choice of ordering is rather unfortunate:
- It is not the common one (most everyone else in the world uses azim-elev-roll, see below)
- It does not correspond to the order in which the 3 rotations take place (that's azim-elev-roll)
- It is not consistent with the ordering in matplotlib's colors.py.
I.e., there is room for improvement.
When I google:
- "azimuth, elevation" -> 291 000 results
- "elevation, azimuth"-> 41 500 results
That is to say, the elevation, azimuth ordering is not the common one. And if we 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 ordering is really a minority position: (41-37)/(291-37) < 2%.
(Also, for what it's worth: MATLAB happens to use "azimuth, elevation", just like (almost) everyone else.)
The ordering with elevation first does not correspond to the order in which the 3 rotations take place:
if thinking about intrinsic rotations, then the rotation order is azim, elev, then roll.
The corresponding quaternion is
q = exp((+roll/2)*e_x)*exp((+elev/2)*e_y)*exp((-azim/2)*e_z)
i.e., to be precise, mplot3d's azim, elev, roll are a kind of Tait-Bryan angles, -z,y',x".
It is a bit of a mess with the Euler angles, apparently there are 24 variations (not counting the signs) - https://github.com/moble/quaternion/wiki/Euler-angles-are-horrible
Proposed fix
Switch (back) to the common ordering, the order in which the rotations are performed: azim, elev, roll.
Fortunately, Scott had the good foresight to use named parameters, so it is not a completely breaking change.
We can make the use of named parameters for azim and elev mandatory (preceding with a '*' argument), so changing the order (when parameters are unnamed) won't result in any silent failures.
Scott voiced a hesitation to change it (again), and I can understand his initial reaction. However, it seems to me that changing the order has clear benefits, and 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 have a PR almost ready to show what the changes would be.
Activity
timhoffm commented on Jun 6, 2024
I assume you are talking about view_init()?
The order "elevation, azimuth" has basically existed forever and not only since #21426. Defining a view direction is also quite a common operation. While users could use kwargs, there are loads of positional calls view_init(elev, azi) out there. https://github.com/search?q=ax.view_init%28+language%3APython+&type=code&p=2
I believe it's not worth forcing changes to all of them.
MischaMegens2 commented on Jun 6, 2024
Yes, view_init(), that's what it comes down to.
The https://github.com/search?q=ax.view_init%28+language%3APython+&type=code&p=2 is interesting -
of the 92 projects I quickly counted there, the breakdown is:
So the 'loads of positional calls' that would be affected are in the minority: 40 would seem manageable.
Not that I would propose to just capriciously change the interface, just like that. But we could nudge in the right direction - update the examples to use the keyword arguments, encourage their use in the documentation (and use the azim, elev order in the mplot3d code itself). Then the next step could be to issue a warning when calling view_init() without using keywords (if the argument values are different). This way you'd have a clear migration path, without breaking anything.
And in a little while the 40 might go down, and we can reconsider.
timhoffm commented on Jun 6, 2024
I interpret these numbers as "almost half the people using this feature are affected". Absolute numbers don't say much. First, only people using that feature are relevant. They'll get the benefits and drawbacks. And while yes, new code is written and that'll get the benefits only, we a large and old enough project that existing code is really important. Second, 40 is by far not a realistic number, there's a lot code out there that is not on GitHub. But assuming the code is otherwise comparable, here "almost half the people using this feature are affected" still holds.
Agreed. This is an improvement anyway. Do you want to make a PR?
Yes, a full migration path is possible, and it would go via a deprecation and kw-only parameters. But this still means, half the users of the feature will have to change their code. And that's what I'm beliving is not worth it.
Note also that new users can use the better way already; it's only that we're not forcing it on them. The only new capability would be eventually being able to use the reversed order positionally, but with proper transition, that'd be at least 3 further releases down the road, and I'm even undecided whether that'd be better than all-positional.
scottshambaugh commented on Jun 8, 2024
I’m -0.5 on this one. The benefits are as @MischaMegens2 mentioned up top, but those are quality-of-life. On the other hand the API change I think is rather annoying for users who are using positional arguments, since no exception will be raised and someone will have to look at and interpret the unexpected view of the plot to understand what went wrong and what to change.
MischaMegens2 commented on Jun 8, 2024
I'll happily make a PR.
Makes sense.
You mean all-keyword, I take it? I'd agree.
MischaMegens2 commented on Jun 8, 2024
Oh I wouldn't suggest messing up the order of positional parameters just like that, I'd just suggest we nudge in the direction of using keyword parameters, preferably in the order azim, elev, roll.
scottshambaugh commented on Jun 9, 2024
Do you mean that we should swap example code to using the kwargs in a new order, but keep the actual function inputs as-is?
MischaMegens2 commented on Jun 10, 2024
Yes, accept the actual function inputs as-is, that seems prudent - then we don't break anything unnecessarily; but we set a good example in the examples (and the rest of the code).
Also, I can imagine not advertising the 'actual function inputs as-is' possibility in the documentation too loudly, and, optionally, issuing a warning that keywords are recommended.
A PR is in the works, but it will take me a few days before it will be ready.
scottshambaugh commented on Jun 10, 2024
I think that would be an ok path for keyword-only arguments, but in this case where these arguments are also positional we should not have in the official docs a different ordering than the function inputs. IMO it would cause more confusion than it helps. Do the other maintainers know of some precedent on this?
Perhaps we can get most the benefit of your second point just by improving the docstrings, writing out the Euler angle rotation interpretation in view_init and/or elsewhere.
MischaMegens2 commented on Jun 14, 2024
A PR is ready: #28395
MischaMegens2 commented on Nov 9, 2024
A new PR is ready: #29114