Skip to content

Add SubplotSpec.add_subplot. #13280

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 our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
wants to merge 1 commit into from

Conversation

anntzer
Copy link
Contributor

@anntzer anntzer commented Jan 24, 2019

Looking for comments about the proposed new API.


Now that the preferred(?) method of creating GridSpecs attaches the
GridSpec to a given figure (figure.add_gridspec), calling
fig.add_subplot(gs[...]) effectively specifies the figure information
twice. Hence, allow one to do

gs = fig.add_gridspec(2, 2)
gs[0, 0].add_subplot()

as a synonym for

gs = fig.add_gridspec(2, 2)
fig.add_subplot(gs[0, 0])

Ideally, this should ultimately allow one to deprecate the somewhat
unwieldy subplot2grid, replacing

plt.subplot2grid((3, 3), (0, 0))
plt.subplot2grid((3, 3), (1, 1), rowspan=2, colspan=2)

by

plt.figure().add_grispec(3, 3)[0, 0].add_subplot()
plt.figure().add_grispec(3, 3)[1:, 1:].add_subplot()

or, after implementing a plt.add_gridspec() that operates on gcf(),

gs = plt.add_gridspec(3, 3)
gs[0, 0].add_subplot()
gs[1:, 1:].add_subplot()

A similar API change would be to make GridSpec.tight_layout() lose its
figure argument (or rather, it could become optional).

PR Summary

PR Checklist

  • Has Pytest style unit tests
  • Code is Flake 8 compliant
  • New features are documented, with examples if plot related
  • Documentation is sphinx and numpydoc compliant
  • Added an entry to doc/users/next_whats_new/ if major new feature (follow instructions in README.rst there)
  • Documented in doc/api/api_changes.rst if API changed in a backward-incompatible way

@jklymak
Copy link
Member

jklymak commented Jan 24, 2019

First, I'd like to see gridspecs as always attached to figures as a logical container for the subplotspecs and their axes. constrained_layout relies on this logical nesting, and while I understand why GridSpec was made more abstract, I don't think it is of practical use to share a gridspec between figures.

However, I'm -0.125 on this suggestion from a readability point of view. You are adding a subplot to the figure, so its a bit dissonant to "add_subplot" from the subplotspec. I guess you are adding and axes?

As for:

plt.figure().add_grispec(3, 3)[0, 0].add_subplot()
plt.figure().add_grispec(3, 3)[1:, 1:].add_subplot()

I'd be pretty against this. It completely loses the relationship between the first axes and the second, and constrained_layout would not be trusted to lay them out properly. The way to do this would be

gs = figure.add_gridspec(3, 3)
fig.add_subplot(gs[0, 0])
fig.add_subplot(gs[1:, 1:])

then all the axes are logical children of the same gridspec.

... not that subplot2grid respects this logic at all, but the point is to properly contain child axes in the same parent so layout can be done.

As I've pointed out in the past, if we had this logical layout throughout (i.e. figure.subplot() respected it), then we could probably make tight_layout work without resorting to the constraint manager in constrained_layout. It'd be tricky, but I think possible. Whether its better than just using a constraint manager is another question.

@anntzer
Copy link
Contributor Author

anntzer commented Jan 24, 2019

First, I'd like to see gridspecs as always attached to figures as a logical container for the subplotspecs and their axes. constrained_layout relies on this logical nesting, and while I understand why GridSpec was made more abstract, I don't think it is of practical use to share a gridspec between figures.

Especially now that there is Figure.add_gridspec, we're in agreement here.

However, I'm -0.125 on this suggestion from a readability point of view. You are adding a subplot to the figure, so its a bit dissonant to "add_subplot" from the subplotspec. I guess you are adding and axes?

Initially I called the method add() or add_self(), but that seemed a bit obscure. No strong opinion on the specific naming.

I'd be pretty against [the subplot2grid replacement suggestion].

Well, as you say it's not really worse than the situation with subplot2grid. I guess my real question is what is really the intended audience of subplot2grid, and whether

gs = plt.add_gridspec(3, 3)
gs[0, 0].add_subplot()
gs[1:, 1:].add_subplot()

can be considered a reasonable pyplot-level replacement for it (not that I'd claim really knowing how to design MATLAB-style interfaces :p but that looks better IMO...).

@anntzer anntzer force-pushed the subplotspec.add_subplot branch from eb3dd25 to 34e84e1 Compare January 24, 2019 20:44
@jklymak
Copy link
Member

jklymak commented Jan 24, 2019

Don’t we have plt.subplot, which I assume can take a subplotspec as an argument? If not, that’s the pyplot change I’d make. I.e


gs = plt.add_gridspec(3, 3)
plt.subplot(gs[0, 0])
plt.subplot(gs[1:, 1:])

I personally don’t think we should tie ourselves up in knots making a Matlab-style interface, because MathWorks has been adding object-oriented features for decades now. I think its a strength that we were object oriented from the start. When I started Matplotlib, I was pretty confused by the Matlab-like interface of the tutorials, but as soon as anything advanced was done a whole new interface was used. It was really hard to tell the canonical way of doing things. If it were up to me, I’d deprecate everything except for plt.subplots, plt.figure and maybe plt.show().

@anntzer
Copy link
Contributor Author

anntzer commented Jan 24, 2019

plt.subplot() already can already take a subplotspec indeed. So perhaps just add add_gridspec to pyplot?
I share your little love for pyplot, but see e.g. #12513 for another PoV...

@jklymak
Copy link
Member

jklymak commented Jan 25, 2019

I'd say adding this convenience method is not going to hurt anything. I am less convinced it should be the canonical way and used extensively in the docs.

I think its mysterious that subplotspec.add_subplot() would add a subplot to a figure associated (way down) with that subplotspec. I think a naive user would (rightly) wonder what the heck is going on. Whereas I think the current canonical way of doing things makes sense.

The new method is also almost completely undiscoverable - I think folks should be able to read Axes and Figure docs in the API guide and get to most of the methods. Here they'd have to get to SubplotSpec, which is something that is only made by a GridSpec call. Thats a pretty deep search to find what we think is the canonical way to add a new subplot to a figure.

@jklymak
Copy link
Member

jklymak commented Jan 25, 2019

Ooops:

So perhaps just add add_gridspec to pyplot?

Could do - I think pyplot tries to make the user ignorant of objects, whereas add_gridspec returns objects, so its mixing paradigms?

@anntzer
Copy link
Contributor Author

anntzer commented Jan 25, 2019

Of course, the "canonical" way may still be add_subplot(x, y, z) (assuming we're not talking about plt.subplots() use-cases), but it is actually not documented in the API that add_subplot() can take a gridspec as argument (#12114).
So from a discoverable documentation point of view, one should add a description of GridSpecs to add_subplot... but then one may just as well point to GridSpec.add_subplot() (or however we want to name it).

Also, I would say that an API that's like

gs = fig.add_gridspec(...)
fig.add_subplot(gs[...])

immediately makes me wonder why you need to specify fig twice, and whether gs[...] can be used on another figure (but perhaps that's just because I look at APIs in a strange way...).

@jklymak
Copy link
Member

jklymak commented Jan 25, 2019

I still think that the user is going to think "I want to add a subplot, how do I specify where it goes?" and we have two ways to do that, and that we should encourage fig.add_subplot as the primary way that happens.

I agree its redundant to specify fig twice, but I don't think that will grate on most people.

Again, not saying this new convenience should not be there, I just don't agree with it as the easiest way to specify a set of gridspecs. But others may disagree.

Copy link
Member

@jklymak jklymak left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So I approve this change, but I don't think all the examples should be changed to use this method. Of course if two other devs would like to see it go in as-is, its not a big deal.

@anntzer
Copy link
Contributor Author

anntzer commented Feb 2, 2019

If the general agreement is to reject this API, that's fine with me too... just waiting for comments from others :)

@jklymak
Copy link
Member

jklymak commented Feb 2, 2019

Just to be clear - I don't reject the API, I just don't think its the most natural and shouldn't necessarily take over the docs.

@anntzer anntzer force-pushed the subplotspec.add_subplot branch from 34e84e1 to 164d0bb Compare February 2, 2019 18:19
@anntzer
Copy link
Contributor Author

anntzer commented Feb 2, 2019

Reverted the examples.

Now that the preferred(?) method of creating GridSpecs attaches the
GridSpec to a given figure (figure.add_gridspec), calling
`fig.add_subplot(gs[...])` effectively specifies the figure information
twice.  Hence, allow one to do
```
gs = fig.add_gridspec(2, 2)
gs[0, 0].add_subplot()
```
as a synonym for
```
gs = fig.add_gridspec(2, 2)
fig.add_subplot(gs[0, 0])
```

Ideally, this should ultimately allow one to deprecate the somewhat
unwieldy subplot2grid, replacing
```
plt.subplot2grid((3, 3), (0, 0))
plt.subplot2grid((3, 3), (1, 1), rowspan=2, colspan=2)
```
by
```
plt.figure().add_grispec(3, 3)[0, 0].add_subplot()
plt.figure().add_grispec(3, 3)[1:, 1:].add_subplot()
```
or, after implementing a plt.add_gridspec() that operates on gcf(),
```
gs = plt.add_gridspec(3, 3)
gs[0, 0].add_subplot()
gs[1:, 1:].add_subplot()
```

A similar API change would be to make GridSpec.tight_layout() lose its
`figure` argument (or rather, it could become optional).
@anntzer anntzer force-pushed the subplotspec.add_subplot branch from 164d0bb to 15e78ae Compare February 2, 2019 21:11
@jklymak
Copy link
Member

jklymak commented Feb 2, 2019

Ha, well, you didn't have to revert them all! I'll approve, but the docs also have to build...

@jklymak jklymak added this to the v3.2.0 milestone Feb 7, 2019
@ImportanceOfBeingErnest
Copy link
Member

I find this API a bit obscure. Logically

object.add_something(where)

makes much more sense than

where.add_something()

@anntzer
Copy link
Contributor Author

anntzer commented Feb 14, 2019

I suggested add() or add_self() above too; not sure it's better.

@ImportanceOfBeingErnest
Copy link
Member

Asking differently: What do we loose when keeping everything as it is?

I can't think of an actual usecase of this, except the one that doesn't work:

gs = gridspec.GridSpec(2,2)
gs[0,1].add_subplot()

So one should probably try to avoid tricking users into attempting that by providing such method.

@anntzer
Copy link
Contributor Author

anntzer commented Feb 14, 2019

I think(?) we now encourage to build gridspecs using fig.add_gridspec rather than invoking the GridSpec constructor directly, which is the case this API supports.

@ImportanceOfBeingErnest
Copy link
Member

So either you have a figure handle as in

gs = fig.add_gridspec(2, 2)
fig.add_subplot(gs[0, 0])

in which case there is no need for the proposed method; or you don't have a figure handle, in which case the proposed method cannot be used either.

@anntzer
Copy link
Contributor Author

anntzer commented Feb 14, 2019

I guess I should have motivated this more.

The use case is when you are simultaneously managing multiple figures, each with its own gridspec and dynamically adding axes.

If you keep track of the axes, you don't need to additionally keep track of the figures, because you can just lookup ax.figure to get the owning figure. For example you can write _, ax = plt.subplots() and not even have a fig variable around.

If you have multiple figures and gridspecs, likewise you(I)'d like to not keep track of both figures and gridspecs (gs = plt.figure().add_gridspec(...) without even bothering with a fig variable). Obviously one could write gs.figure.add_subplot(gs[...]), but gs[...].add_subplot() just seems much less redundant.

@timhoffm
Copy link
Member

I'm -0.5 on this.

  1. I've an odd feeling on adding yet another way to add a subplot.

  2. The canonical use case

gs = fig.add_gridspec(2, 2)
fig.add_subplot(gs[0, 0])

is already straight forward and doesn't benefit from the proposed

gs = fig.add_gridspec(2, 2)
gs[0, 0].add_subplot()

In contrast, it makes it more implicit, and doesn't reflect the actual object relation: The subplot belongs to the figure, not to the gridspec.

  1. IMO the dynamic case with multiple figures is rare. In itself it does not justify adding additional API. Additionally, gs.figure.add_subplot(gs[...]) is a bit verbose, but also more explicit than gs[...].add_subplot().

  2. pyplot is not a strong argument either. I don't think it needs to expose the full API and is mostly used for simpler plots. Nevertheless you could

gs = plt.add_gridspec(3, 3)
plt.add_subplot(gs[0, 0])
plt.add_subplot(gs[1:, 1:])

working on gcf() which is even more in the spirit of "no objects" compared to gs[0, 0].add_subplot().

@anntzer
Copy link
Contributor Author

anntzer commented Mar 17, 2019

Let's just close this.

@anntzer anntzer closed this Mar 17, 2019
@anntzer anntzer deleted the subplotspec.add_subplot branch March 17, 2019 16:27
@anntzer anntzer mentioned this pull request Nov 30, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants