[MNT]: mplot3d azim-elev-roll order #28353
Comments
|
I assume you are talking about 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 I believe it's not worth forcing changes to all of them. |
Yes, The https://github.com/search?q=ax.view_init%28+language%3APython+&type=code&p=2 is interesting -
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 |
|
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. |
|
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. |
I'll happily make a PR.
Makes sense.
You mean all-keyword, I take it? I'd agree. |
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 |
|
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? |
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). |
|
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 |
A PR is ready: #28395 |
|
A new PR is ready: #29114 |
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 everywhereHowever, the choice of ordering is rather unfortunate:
colors.py.I.e., there is room for improvement.
When I google:
"azimuth, elevation"-> 291 000 results"elevation, azimuth"-> 41 500 resultsThat is to say, the
elevation, azimuthordering 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, azimuthordering 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, rollare 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
azimandelevmandatory (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.
The text was updated successfully, but these errors were encountered: