Uh oh!
There was an error while loading.Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork7.9k
Prepare for merging SubplotBase into AxesBase.#18564
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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Uh oh!
There was an error while loading.Please reload this page.
Conversation
917e301 to8318108CompareThere was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
This all looks good to me. You could up the test coverage a bit with a some trivial new tests of your 4 new methods. And the test_figure change you made is really testing something different. But, I think thats all minor.
When all done, I think this will be a significant simplification....
| @@ -170,7 +170,7 @@ def test_gca(): | |||
| # Changing the projection will throw a warning | |||
| assert fig.gca(polar=True) is not ax3 | |||
| assert fig.gca(polar=True) is not ax2 | |||
| assert fig.gca().get_geometry() == (1, 1,1) | |||
| assert fig.gca().get_subplotspec().get_geometry() == (1, 1,0, 0) | |||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
should you check that the subplot spec is 1, 1, 1?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
I think that's an equivalent check?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Ooops, I see. You are indeed correct...
is_first_row/is_last_col are not tested by the test suite (that hasn't changed from before the PR), but there are actually examples/tutorials which exercise it so they are actually already getting run. |
Do we have any idea why we have this split anymore? |
I think before, it was harder to get back to a subplot's subplot spec and gridspec. But we've added that tracery now, so we don't need to carry that info around explicitly on the axes? |
c3de3b6 suggests it was to support |
lib/matplotlib/axes/_subplots.py Outdated
| @@ -49,12 +47,13 @@ def __reduce__(self): | |||
| (axes_class,), | |||
| self.__getstate__()) | |||
| @cbook.deprecated("3.4", alternative="get_subplotspec") | |||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
The change in the return type needs to be flagged.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
sure
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
I am 👍 to the changes, but want to either handle the 3D axes API change note here or get that merged first.
Anyone can clear this.
It should be possible to merge SubplotBase into AxesBase, with all Axeshaving a `get_subplotspec()` method -- it's just that that method wouldreturn None for non-gridspec-positioned Axes. The advantage of doing sois that we could get rid of the rather hackish subplot_class_factory,and the dynamically generated AxesSubplot class, which appears nowherein the docs.- Deprecate most Subplot-specific API: while it's fine for all axes to have a `get_subplotspec` which may return None, it would be a bit weird if they all also have e.g. a `is_last_row` for which it's not clear what value to return for non-gridspec-positioned Axes. Moving that to the SubplotSpec seems natural enough (and these are pretty low-level anyways).- Make most parameters to AxesBase keyword-only, so that we can later overload the positional parameters to be either a rect or a subplot triplet (which should ideally be passed packed as a single parameter rather than unpacked, but at least during the deprecation period it would be a pain to differentiate whether, in `Axes(fig, a, b, c)`, `b` was intended to be the second index of a subplot triplet or a `facecolor`...)Due to the order of calls during initialization, Axes3D self-adding tothe figure was problematic. This is already getting removed in anotherPR, so I included the same change here without API changes notes just toget the tests to pass. However I can put a note in for this PR if itends up being ready for merge first.
| @@ -131,8 +131,6 @@ def __init__( | |||
| pseudo_bbox = self.transLimits.inverted().transform([(0, 0), (1, 1)]) | |||
| self._pseudo_w, self._pseudo_h = pseudo_bbox[1] - pseudo_bbox[0] | |||
| self.figure.add_axes(self) | |||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Is this going to break basically every usage of 3D axes?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
No, only those using the rather unidiomaticAxes3D(fig) instead offig.add_subplot(..., projection="3d").
Uh oh!
There was an error while loading.Please reload this page.
It should be possible to merge SubplotBase into AxesBase, with all Axes
having a
get_subplotspec()method -- it's just that that method wouldreturn None for non-gridspec-positioned Axes. The advantage of doing so
is that we could get rid of the rather hackish subplot_class_factory,
and the dynamically generated AxesSubplot class, which appears nowhere
in the docs. (See discussion at#18222.)
Deprecate most Subplot-specific API: while it's fine for all axes to
have a
get_subplotspecwhich may return None, it would be a bitweird if they all also have e.g. a
is_last_rowfor which it's notclear what value to return for non-gridspec-positioned Axes. Moving
that to the SubplotSpec seems natural enough (and these are pretty
low-level anyways).
Make most parameters to AxesBase keyword-only, so that we can later
overload the positional parameters to be either a rect or a
subplot triplet (which should ideally be passed packed as a single
parameter rather than unpacked, but at least during the deprecation
period it would be a pain to differentiate whether, in
Axes(fig, a, b, c),bwas intended to be the second index of a subplot tripletor a
facecolor...)Due to the order of calls during initialization, Axes3D self-adding to
the figure was problematic. This is already getting removed in another
PR (#18356), so I included the same change here without API changes notes just to
get the tests to pass. However I can put a note in for this PR if it
ends up being ready for merge first.
PR Summary
PR Checklist
pytestpasses).flake8on changed files to check).flake8-docstringsandpydocstyle<4and runflake8 --docstring-convention=all).doc/users/next_whats_new/(follow instructions in README.rst there).doc/api/next_api_changes/(follow instructions in README.rst there).