Change order of mplot3d view angles to match order of rotations #28395
Conversation
|
An option may be to make these keyword only. This will introduce a smoother translation path, but take longer time. Once they are keyword only, the order doesn't matter... (So can be changed in the code base without any problems, nor benefits...) Edit: I'm not up to date with the discussion, so maybe this has been discussed... |
Has been 😄 #28353 (comment) |
|
Ready for review again. |
|
Anything else needed for a merge? (Does that python 3.9 on macos-12 need to be revived?) (Extra 0.13% code coverage?) (More reviewers?) |
f4e4880 to
953177e
Compare
|
Added a test to improve coverage, and rebased. Ready for merge (I don't think the remaining failing checks have anything to do with the PR). |
|
ping @scottshambaugh Could you take a look at it, or ask someone else for additional review (timhoffm?) |
|
Hey @MischaMegens2, it's got my approval. For MRs we require approvals from 2 maintainers and active consensus if there is disagreement before they can be merged in. It can be a little frustrating to have good-to-go MRs sitting waiting for a bit unlooked at, but since everyone here is volunteering time that's for better or worse the nature of the beast, and it can take some time for someone to get around to it. Right now especially it doesn't really help that the main branch CI is failing, so it's not so easy for us to scan the MRs and see which ones have all the checks passing. If you want to prod it along, especially if an MR is blocking a future improvement you want to work on, there are basically 3 avenues to do so:
In this case since @timhoffm was active on the original issue, he's probably a good one to ping for a review here. |
|
Ping @timhoffm Dear Tim, would you be able to take a look and provide a second review? |
|
I'm not convinced of using a different order for keyword arguments than their positional definition. It's a common expectation that parameters are listed in the order they are defined in the signature. When seeing the following both notations, it's not uncommon to assume that -35 in the second example will be Since users will see both notations, or try to shorten from the first to the second to save typing, I believe we're doing more harm than good by reordering the official docs. I'm sorry, I have not read/thought carefully enough here
I believe we should use kwargs throughout to make the "unconventional order" transparent. But we should not try to gloss over it by reordering the arguments when calling. Since we cannot change the order in the forseeable future, I'd rather have users slightly surprised by seeing the "unconventional order", rather than making them believe everything is conventional and then stumble over positional order. That's a didactic aspect of our docs. Of course, users are free to use any order they like in their code. |
|
@timhoffm: Do I understand correctly then that
(The change to the examples should come later?)
Well yes, it would be an assumption, and an unwarranted assumption - as you said, people are free to use any order they like for keyword arguments...
Such shortening would be ill-advised of course; but I don't quite understand why that would be desirable, what's the appeal. 'There is no tax on keystrokes' (Bertrand Meyer).
So I thought we are at liberty to do the two steps above simultaneously, without introducing grave risks for mistakes. |
|
Oh, no, communication has gone quite wrong here. My appologies, I have not been reading and writing carefully enough (too much going on and trying to quickly respond in between 😢). My standpoint is that there is so much current positional use that we cannot afford to bother existing users with changes. With any such change, we would impose work on current users. That also includes a runtime warning for positional use. While not breaking, users would still have to modify their code, because you don't want to have code with warnings. The only thing we should do, is change our internal examples to use kwargs, to make the users aware of the meaning and order of the parameters. Here, I would still use the order as in the signature to avoid implicit surprises: If really wanted we could add a note to the docstring:
But I'm 50/50 on that (one can also make things more complicated than they are). Unfortunately, this also means there is no migration path and we'll have to live with the unconventional order. It's a tradeoff between a suboptimal API and backward compatibility; for this case, I'm clearly on the side of not annoying existing users. You would have to convince other core developers that this change is worth the churn. |
There was a problem hiding this comment.
I block this based on #28395 (comment)
If others think this is an acceptable change, please review.
I've come back around to agreeing with Tim on this one
Ah, I can probably come up with a less disruptive approach, shortly. |
953177e to
89a4d55
Compare
|
(The remaining CircleCI warnings are: Not quite sure what to do about those, please advise...) |
You need to use double backticks for literal quoting. Single backticks try to link to code objects, which don't exist and thus warn. |
There was a problem hiding this comment.
Global comment: we have a fundamental discrepancy in angle order between our API (which as discussed we cannot change) and the common convention "out there". For each usage we have to decide, which one we want to be consistent with. I favor internal consistency. In degrees of emphasis
-1 on order changes in public API
-0.5 on order changes non-public code
-0.1 on order changes in non-code changes (e.g. written documentation)
| the order of operations. First the camera is rotated about the scene's +x axis | ||
| (*roll*), then the +y axis (*elev*), then the −z axis (*azim*). | ||
|
|
||
| If you would like to make the connection with quaternions (because |
There was a problem hiding this comment.
I suggest to make this whole block a .. note::
There was a problem hiding this comment.
I considered it, but I get the impression that ..note:: is not meant for the purpose of adding some reference material which is a bit secondary, as it seems is our intention; rather, it is for information you want the user to pay particular attention to. I wouldn't want to put that much emphasis on the particulars of the definition of the view angles.
Since the text already mentions that '[The view angles] are further documented in the .mplot3d.axes3d.Axes3D.view_init API', I thought it be sensible to move the information there (and keep the view_angles.rst as it was). I'll give it a try.
| roll = np.deg2rad(self.roll) | ||
| q = _Quaternion.from_cardan_angles(elev, azim, roll) | ||
| q = _Quaternion.from_cardan_angles(azim, elev, roll) |
There was a problem hiding this comment.
This is not the order that the function API defines. Please be very careful what you change.
There was a problem hiding this comment.
...well actually, the order is the order that the _Quaternion API defines, and the change is as intended. Since the quaternion class has its own API, it is not bound by my previous mistake to force it to be consistent with the Axes3D class. And furthermore, _Quaternion is an internal class, so I have little hesitation to change it back to the way it was initially, with a logical progression of arguments for from_cardan_angles() and as_cardan_angles().
| It is recommended to provide the 'view angles' for three-dimensional plots | ||
| (with ``mplot3d``) as keyword arguments | ||
| ``.view_init(azim=..., elev=..., roll=..., ...)`` | ||
| in this order. This is consistent with the order in which the rotations take |
There was a problem hiding this comment.
This is outdated. We do not want to change the order. I believe the whole waht's new note should go. Instead put a note on the unconventional order in view_angles.rst and/or the view_init docstring.
Implement changes requested for PR matplotlib#28395: - remove incohesive section from view_angles.rst - move some of the information to .mplot3d.axes3d.Axes3D.view_init - remove redundant kwargs as in .view_init(elev=elev, azim=azim, roll=roll) - cleanup test_axes3d_primary_views() - remove outdated next_whats_new item
Change order of mplot3d view angles to azim-elev-roll, the order in which rotations occur, but keep the elev, azim positional order in view_init(), for backwards compatibility; use keyword arguments throughout, to avoid confusion: - in axes3d.py - in tests - in documentation - in examples Implement changes requested for PR matplotlib#28395: - remove incohesive section from view_angles.rst - move some of the information to .mplot3d.axes3d.Axes3D.view_init - remove redundant kwargs as in .view_init(elev=elev, azim=azim, roll=roll) - cleanup test_axes3d_primary_views() - remove outdated next_whats_new item
e9991b3 to
511ad71
Compare
|
@timhoffm: Should be good to go, please take a look. (I left the 'Resolve conversation' up to you.) |
There was a problem hiding this comment.
After looking at this again, I think the changes here make things more confusing than not. There are so many Euler/cardan angle orderings for 3D orientations out there, that confirming a specific convention before use is expected. IMO the pedagogical burden is far less for users by giving them complete internal consistency rather than partially aligning with rotation ordering used elsewhere. I think action to make things clearer should focus on improving the docs and examples, without changing the keyword argument ordering.
Greatly appreciate the effort and thought that went into this PR, but I'm a -1 on merging it in.
| @@ -16,13 +16,13 @@ def annotate_axes(ax, text, fontsize=18): | |||
| ax.text(x=0.5, y=0.5, z=0.5, s=text, | |||
| va="center", ha="center", fontsize=fontsize, color="black") | |||
|
|
|||
| # (plane, (elev, azim, roll)) | |||
There was a problem hiding this comment.
This change removes the ability to copy-paste these docs into one's functions, and is confusing wrt the keyword ordering below.
|
I argee with @scottshambaugh. Let's prioritize internal consistency within Matplotlib. |
How about: I revise it to just clarifying the situation in the documentation? |
|
Pushed tentatively to 3.11 as we are trying to close out 3.10, though if (as it sounds like) this turns into doc-only changes, then certainly willing to put into the 3.10.x-doc branch. |
@ksunden: Yes, doc-only changes. What is the time frame for 3.10.x-doc ? |
|
Doc changes are merged when they are ready, don't necessarily follow the release schedule. I intend to branch soon, but essentially doc only means it can be backported even without a patch release being needed. It's mostly a distinction for maintainers. If this PR were to be included in its current state, which changes parameter orders, etc. It would likely land in the next |
If this contains docstring changes on anything in https://matplotlib.org/devdocs/devel/pr_guide.html#backport-strategy |
Oh, it would contain docstring changes in |
|
There is a new PR #29114 to replace this one, let's close this one. |
PR summary
Changes the order of the view angles to azim-elev-roll, since:
The proposed changes (to the documentation of view_init() in particular) suggest to use keyword arguments
in this order, but the historical positional arguments are still accepted without change, so the interface does not actually change with this PR.
Resolves #28353.
Summary of changes:
PR checklist