ENH: reuse oldgridspec is possible... #17347

jklymak · 2020-05-06T23:58:49Z

PR Summary

A major constraint of constrained_layout is that it never worked with add_subplot(2,3,1) because a new gridspec was created each time. This PR causes the gridspec to be reused if nrows and ncols are the same.

Replaces #11441.

I decided that the grid specs should be exactly the same, so add_subplot(4, 4, 2) would be a different grid than add_subplot(2, 2, 4) despite the fact they could theoretically be made the same. Basically if you want those two axes to be in the same layout, you should specify them as
add_subplot(4, 4, 2) and add_subplot(4, 4, (11, 16))

Update: Now works with subplotgrid as well.

Closes #15821

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 · 2020-05-26T04:49:49Z

@anntzer you looked at a previous version of this, so if you had time ...

anntzer · 2020-05-26T06:30:42Z

tutorials/intermediate/constrainedlayout_guide.py

-ax3 = plt.subplot(122)
+ax1 = plt.subplot(2, 2, 1)
+ax2 = plt.subplot(2, 2, 3)
+ax3 = plt.subplot(2, 2, (2, 4))


perhaps add a comment explaining what this does?

anntzer · 2020-05-26T06:31:43Z

lib/matplotlib/gridspec.py

+                    # This is probably OK becase this whole logic tree
+                    # is for when the user is doing simple things with the
+                    # add_subplot command.  Complicated stuff, the proper
+                    # gridspec is passed in...


perhaps further add a comment that this is only for cases like subplot(ijk) where the user cannot be explicitly passing in e.g. width_ratios or height_ratios, so we don't need to try to match these.

Clarified to

# For complicated layouts like subgridspecs the proper gridspec is passed in...

I think width_ratios etc are not the problem, but nested gridspecs are not particularly handled here. But I don't know how you add a subplot to a nested gridspec without grabbing its subplotspec, so I think this is fine.

anntzer

Just some clarifications suggested, but lgtm. Dunno if it's worth a whatsnew.

jklymak · 2020-05-26T14:30:04Z

...ooops, yes it needs a Whats new.

anntzer · 2020-05-26T15:20:55Z

lib/matplotlib/gridspec.py

+                    ggs = ggs.get_topmost_subplotspec().get_gridspec()
+                (nrow, ncol) = ggs.get_geometry()
+                if nrow == nrows and ncol == ncols:
+                    gs = ggs


perhaps you can just return ggs here? (dunno if it matters whether you pick the first or the last match, hopefully there's only one anyways...) and then you don't need to have gs = None at the top, yada yada.

tacaswell · 2020-07-13T19:48:37Z

I am 👍 in principle, have not read the code carefully. I support this being merged on 1 very careful reviewer.

efiring

Clarifications and a couple condensations...

efiring · 2020-07-13T20:57:25Z

doc/users/next_whats_new/2020-05-26-cl-subplot.rst

+as the top level gridspec already in the figure.  i.e. ``plt.subplot(2, 1, 2)``
+will use the same gridspec as ``plt.subplot(2, 1, 1)`` and the
+``constrained_layout=True`` option to `~.figure.Figure` will work.  Note that
+mixing ``nrows`` and ``ncols`` will *not* work: ``plt.subplot(2, 2, 1)``


The expression "Note that mixing nrows and ncols will not work:..." is confusing; it would be clearer here if you just said, "In contrast, ..."

efiring · 2020-07-13T20:57:48Z

doc/users/next_whats_new/2020-05-26-cl-subplot.rst

+mixing ``nrows`` and ``ncols`` will *not* work: ``plt.subplot(2, 2, 1)``
+followed by ``plt.subplots(2, 1, 2)`` will produce two gridspecs, and
+``constrained_layout=True`` will give bad results.  In order to get the
+same effect, the second call can specify the cells the second axes is meant


"same" -> "desired"

efiring · 2020-07-13T21:02:05Z

lib/matplotlib/gridspec.py

+                    # like subgridspecs the proper gridspec is passed in...
+                    gs = gs.get_topmost_subplotspec().get_gridspec()
+                (nrow, ncol) = gs.get_geometry()
+                if nrow == nrows and ncol == ncols:


Optional: 2 lines could be condensed to

if gs.get_geometry() == (nrows, ncols):

efiring · 2020-07-13T21:04:09Z

lib/matplotlib/gridspec.py

                        f"integer, not {arg}") from None
                # num - 1 for converting from MATLAB to python indexing
-                return GridSpec(rows, cols, figure=figure)[num - 1]
+                i = j = num


It looks like the comment above this line no longer belongs there.

efiring · 2020-07-13T21:09:33Z

lib/matplotlib/gridspec.py

                        f"num must be 1 <= num <= {rows*cols}, not {num}")
-                return gs[num - 1]   # -1 due to MATLAB indexing.
+                else:
+                    i = j = num


You can delete the "else" and unindent the line.

efiring · 2020-07-13T21:16:06Z

doc/users/next_whats_new/2020-05-26-cl-subplot.rst

+``constrained_layout=True`` will give bad results.  In order to get the
+same effect, the second call can specify the cells the second axes is meant
+to cover:  ``plt.subplots(2, 2, (2, 4))``, or the more pythonic
+``plt.subplots2grid((2, 2), (0, 1), rowspan=2)`` can be used.


"subplots2grid" -> "subplot2grid"

efiring · 2020-07-13T21:22:18Z

doc/users/next_whats_new/2020-05-26-cl-subplot.rst

+
+Previously ``constrained_layout`` depended on a single ``GridSpec``
+for each logical layout on a figure, whereas ``plt.subplot`` added
+a new ``GridSpec`` for each call.  Now ``plt.subplot`` attempts to


Constrained layout still depends on a single Gridspec, doesn't it? Maybe change to "Previously, plt.subplot added a new GridSpec with each call, but constrained layout depends on working with a single GridSpec for...figure. Now, plt.subplot..."

efiring · 2020-07-13T21:23:57Z

doc/users/next_whats_new/2020-05-26-cl-subplot.rst

@@ -0,0 +1,17 @@
+Constrained layout now supports subplot and subplot2grid


Or, "Subplot and subplot2grid can now work with constrained layout". (I used "can" because they work only if called appropriately.)

jklymak · 2020-07-13T21:47:45Z

Thanks @efiring. Made the small changes and largely rewrote the what's new based on your suggestions.

jklymak mentioned this pull request May 6, 2020

ENH: make subplot reuse gridspecs if they fit #11441

Closed

6 tasks

jklymak added this to the v3.4.0 milestone May 7, 2020

jklymak added the topic: geometry manager LayoutEngine, Constrained layout, Tight layout label May 7, 2020

jklymak force-pushed the enh-reuse-oldgrdspec branch from 3289ff7 to 3d9fbcc Compare May 7, 2020 02:52

jklymak mentioned this pull request May 23, 2020

Should constrained_layout work as plt.figure() argument? #15821

Closed

jklymak force-pushed the enh-reuse-oldgrdspec branch from ca184e9 to 93142c6 Compare May 25, 2020 17:51

jklymak marked this pull request as ready for review May 25, 2020 19:18

jklymak force-pushed the enh-reuse-oldgrdspec branch from c4225f3 to b0b9792 Compare May 25, 2020 19:19

jklymak requested a review from anntzer May 26, 2020 04:49

anntzer reviewed May 26, 2020

View reviewed changes

anntzer approved these changes May 26, 2020

View reviewed changes

jklymak force-pushed the enh-reuse-oldgrdspec branch from b0b9792 to e318138 Compare May 26, 2020 14:41

anntzer reviewed May 26, 2020

View reviewed changes

jklymak force-pushed the enh-reuse-oldgrdspec branch from e318138 to eeaf3e2 Compare May 26, 2020 15:29

jklymak force-pushed the enh-reuse-oldgrdspec branch from 2df24aa to 126f6a8 Compare July 13, 2020 15:49

jklymak added the status: merge with single review? label Jul 13, 2020

efiring requested changes Jul 13, 2020

View reviewed changes

ENH: reuse gridspec if possible

487ac80

jklymak force-pushed the enh-reuse-oldgrdspec branch from 126f6a8 to 487ac80 Compare July 13, 2020 21:47

efiring approved these changes Jul 13, 2020

View reviewed changes

efiring merged commit db3ecd2 into matplotlib:master Jul 13, 2020

QuLogic removed the status: merge with single review? label Apr 1, 2021

		@@ -0,0 +1,17 @@
		Constrained layout now supports subplot and subplot2grid

Uh oh!

ENH: reuse oldgridspec is possible... #17347

ENH: reuse oldgridspec is possible... #17347

Uh oh!

Conversation

jklymak commented May 6, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

PR Summary

PR Checklist

Uh oh!

jklymak commented May 26, 2020

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

anntzer left a comment

Choose a reason for hiding this comment

Uh oh!

jklymak commented May 26, 2020

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tacaswell commented Jul 13, 2020

Uh oh!

efiring left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jklymak commented Jul 13, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

jklymak commented May 6, 2020 •

edited

Loading