matplotlib
Change order of mplot3d view angles to match order of rotations
PR summary
Changes the order of the view angles to azim-elev-roll, since:
- azim-elev(-roll) appears to be a much more common order, the elev-azim(-roll) that we had is unusual
- it corresponds to the order in which the 3 rotations take place
- it is consistent with the ordering in matplotlib's colors.py
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:
- Change order of mplot3d view angles to azim-elev-roll, the order in which rotations occur:
- in axes3d.py
- in tests
- in documentation
- in examples
- Add description of rotation to view_angles.rst
- Put in ref. to https://github.com/moble/quaternion/wiki/Euler-angles-are-horrible
- Update view_init() documentation
- Add doc\api\next_api_changes\deprecations warning that Axes3D.view_init preferably should be used with keyword arguments
- Add next_whats_new\view_angles_order.rst explaining the new view angles order
PR checklist
- [x] "closes #0000" is in the body of the PR description to link the related issue
- [x] new and changed code is tested
- [N/A] Plotting related features are demonstrated in an example
- [(please review)] New Features and API Changes are noted with a directive and release note
- [x] Documentation complies with general and docstring guidelines
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...
Edit: I'm not up to date with the discussion, so maybe this has been discussed...
Has been 😄 https://github.com/matplotlib/matplotlib/issues/28353#issuecomment-2153423466
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?)
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:
- Request a review from someone in the top right menu, or @ them directly
- Ask for a review on the project chat on gitter: https://app.gitter.im/#/room/#matplotlib_matplotlib:gitter.im
- Hop on the weekly zoom meeting and bring it up: https://hackmd.io/l9vkn_T4RSmk147H_ZPPBA
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 azim.
ax.view_init(azim=-35, elev=20)
ax.view_init(-35, 20)
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
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).
Agreed. This is an improvement anyway. Do you want to make a PR?
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
view_init()suggestion is fine in your opinion, but - the changing of the order in the examples you think is a bridge too far at this point
(The change to the examples should come later?)
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
azim.ax.view_init(azim=-35, elev=20) ax.view_init(-35, 20)
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...
Since users will see both notations, or try to shorten from the first to the second to save typing, [...]
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). Fortunately, if anyone did so inadvertently, then they might stumble momentarily but wouldn't fall, they would be saved by:
- Add doc\api\next_api_changes\deprecations warning that Axes3D.view_init preferably should be used with keyword arguments.
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: ax.view_init(azim=-35, elev=20).
If really wanted we could add a note to the docstring:
It's recommended to provide the parameter as keyword arguments
.view_init(elev=20, azim=-35). The positional order is unfortunately different from that used in other programs(azim, elev), but cannot be changed du to backward compatibility.
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.
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:
ax.view_init(azim=-35, elev=20).
Ah, I can probably come up with a less disruptive approach, shortly.
(The remaining CircleCI warnings are:
/home/circleci/project/lib/mpl_toolkits/mplot3d/axes3d.py:docstring of mpl_toolkits.mplot3d.axes3d.Axes3D.view_init:23: WARNING: py:obj reference target not found: azim, elev
/home/circleci/project/doc/users/next_whats_new/view_angles_keyword_arguments.rst:4: WARNING: py:obj reference target not found: elev, azim
Not quite sure what to do about those, please advise...)
(The remaining CircleCI warnings are:
/home/circleci/project/lib/mpl_toolkits/mplot3d/axes3d.py:docstring of mpl_toolkits.mplot3d.axes3d.Axes3D.view_init:23: WARNING: py:obj reference target not found: azim, elev /home/circleci/project/doc/users/next_whats_new/view_angles_keyword_arguments.rst:4: WARNING: py:obj reference target not found: elev, azimNot 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.
@timhoffm: Should be good to go, please take a look. (I left the 'Resolve conversation' up to you.)
I argee with @scottshambaugh. Let's prioritize internal consistency within Matplotlib.
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.
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 .0 (meso) release.
essentially doc only means it can be backported even without a patch release being needed.
If this contains docstring changes on anything in lib, it doesn't qualify for the doc branch and would be targeted for the next micro release 3.10.x.
https://matplotlib.org/devdocs/devel/pr_guide.html#backport-strategy
If this contains docstring changes on anything in
lib, it doesn't qualify for the doc branch and would be targeted for the next micro release 3.10.x.
Oh, it would contain docstring changes in lib (of course), good to know!