Prepare for merging SubplotBase into AxesBase. #18564

anntzer · 2020-09-24T20:44:08Z

It should be possible to merge SubplotBase into AxesBase, with all Axes
having a get_subplotspec() method -- it's just that that method would
return 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_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 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

Has pytest style unit tests (and pytest passes).
Is Flake 8 compliant (run flake8 on changed files to check).
New features are documented, with examples if plot related.
Documentation is sphinx and numpydoc compliant (the docs should build without error).
Conforms to Matplotlib style conventions (install flake8-docstrings and pydocstyle<4 and run flake8 --docstring-convention=all).
New features have an entry in doc/users/next_whats_new/ (follow instructions in README.rst there).
API changes documented in doc/api/next_api_changes/ (follow instructions in README.rst there).

jklymak

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....

jklymak · 2020-09-24T23:46:49Z

lib/matplotlib/tests/test_figure.py

@@ -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)


should you check that the subplot spec is 1, 1, 1?

I think that's an equivalent check?

Ooops, I see. You are indeed correct...

anntzer · 2020-09-25T00:37:58Z

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.

tacaswell · 2020-09-25T04:31:59Z

Do we have any idea why we have this split anymore?

jklymak · 2020-09-25T04:34:06Z

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?

anntzer · 2020-09-25T09:10:27Z

c3de3b6 suggests it was to support Axes([l, r, w, h]) and Subplot(1, 1, 1) but also for polar plots, but we rarely explicitly instantiate Axes (on the user side) anyways, and we can always overload the constructor (once facecolor becomes kwonly).

tacaswell · 2020-09-25T14:21:20Z

lib/matplotlib/axes/_subplots.py

@@ -49,12 +47,13 @@ def __reduce__(self):
                (axes_class,),
                self.__getstate__())

+    @cbook.deprecated("3.4", alternative="get_subplotspec")


The change in the return type needs to be flagged.

tacaswell

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 Axes having a `get_subplotspec()` method -- it's just that that method would return 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. - 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 to the figure was problematic. This is already getting removed in another PR, 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.

added whatsnew

tacaswell · 2020-09-25T17:31:39Z

lib/mpl_toolkits/mplot3d/axes3d.py

@@ -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)


Is this going to break basically every usage of 3D axes?

No, only those using the rather unidiomatic Axes3D(fig) instead of fig.add_subplot(..., projection="3d").

anntzer added this to the v3.4.0 milestone Sep 24, 2020

anntzer force-pushed the ss branch 2 times, most recently from 917e301 to 8318108 Compare September 24, 2020 21:19

jklymak approved these changes Sep 24, 2020

View reviewed changes

jklymak reviewed Sep 24, 2020

View reviewed changes

tacaswell reviewed Sep 25, 2020

View reviewed changes

anntzer force-pushed the ss branch from 8318108 to 896013c Compare September 25, 2020 14:39

tacaswell previously requested changes Sep 25, 2020

View reviewed changes

anntzer force-pushed the ss branch from 896013c to 261f706 Compare September 25, 2020 16:47

tacaswell reviewed Sep 25, 2020

View reviewed changes

tacaswell merged commit 5173cda into matplotlib:master Sep 25, 2020

anntzer deleted the ss branch September 25, 2020 19:32

This was referenced Nov 12, 2020

Warn re: Axes3D constructor behavior change in mpl3.4 #18939

Closed

It is painful as a new user, to figure out what AxesSubplot is #18222

Closed

NicolasGensollen mentioned this pull request Nov 20, 2020

Axes3D no longer adds itself to figure nilearn/nilearn#2614

Closed

jklymak mentioned this pull request Feb 1, 2021

FIX: allow add option for Axes3D(fig) #19413

Closed

7 tasks

jklymak mentioned this pull request Oct 23, 2021

[Bug]: axes(position = [...]) behavior #21441

Closed

anntzer mentioned this pull request Aug 6, 2022

Merge SubplotBase into AxesBase. #23573

Merged

6 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Prepare for merging SubplotBase into AxesBase. #18564

Prepare for merging SubplotBase into AxesBase. #18564

anntzer commented Sep 24, 2020 •

edited

Loading

jklymak left a comment

jklymak Sep 24, 2020

anntzer Sep 25, 2020

jklymak Sep 25, 2020

anntzer commented Sep 25, 2020

tacaswell commented Sep 25, 2020

jklymak commented Sep 25, 2020

anntzer commented Sep 25, 2020

tacaswell Sep 25, 2020

anntzer Sep 25, 2020

tacaswell left a comment

tacaswell Sep 25, 2020

anntzer Sep 25, 2020

Prepare for merging SubplotBase into AxesBase. #18564

Prepare for merging SubplotBase into AxesBase. #18564

Conversation

anntzer commented Sep 24, 2020 • edited Loading

PR Summary

PR Checklist

jklymak left a comment

Choose a reason for hiding this comment

jklymak Sep 24, 2020

Choose a reason for hiding this comment

anntzer Sep 25, 2020

Choose a reason for hiding this comment

jklymak Sep 25, 2020

Choose a reason for hiding this comment

anntzer commented Sep 25, 2020

tacaswell commented Sep 25, 2020

jklymak commented Sep 25, 2020

anntzer commented Sep 25, 2020

tacaswell Sep 25, 2020

Choose a reason for hiding this comment

anntzer Sep 25, 2020

Choose a reason for hiding this comment

tacaswell left a comment

Choose a reason for hiding this comment

tacaswell Sep 25, 2020

Choose a reason for hiding this comment

anntzer Sep 25, 2020

Choose a reason for hiding this comment

anntzer commented Sep 24, 2020 •

edited

Loading