ENH: Only do constrained layout at draw... by jklymak · Pull Request #20229 · matplotlib/matplotlib

jklymak · 2021-05-14T05:37:28Z

PR Summary

This PR moves all of the state for constrained_layout out of figure and gridspec except for the kwargs in figure. There are a few clear advantages.

No extra state to worry about if the figure is serialized (kiwi solver objects don't serialize).
constrained_layout can respond to changing gridspec specs (like width ratios) because all the state is decided at draw time.
More parallel to tight_layout which does all its work at draw time as well.

This may have a minor speed hit because all the layout boxes are made each draw, but I doubt its very significant

performance benchmarks.
- seems to be no discernible difference using the mpl-bench constrained_layout benchmarks...

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 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 · 2021-05-14T14:42:33Z

Benchmarks

https://github.com/matplotlib/mpl-bench

% asv dev --bench constrained

master:

asv dev --bench constrained
· Discovering benchmarks
· Running 2 total benchmarks (1 commits * 1 environments * 2 benchmarks)
[  0.00%] ·· Benchmarking existing-py_Users_jklymak_anaconda3_envs_matplotlibdev_bin_python
[ 25.00%] ··· constrained_layout.time_subplots                                                                                                                  ok
[ 25.00%] ··· === ==========
               n
              --- ----------
               1   55.3±0ms
               2   184±0ms
               5   833±0ms
              === ==========

[ 50.00%] ··· constrained_layout.time_subplots_colorbar                                                                                                         ok
[ 50.00%] ··· === =========
               n
              --- ---------
               1   134±0ms
               2   448±0ms
               4   1.34±0s
              === =========

This PR:

[ 25.00%] ··· constrained_layout.time_subplots                                                                                                                  ok
[ 25.00%] ··· === ==========
               n
              --- ----------
               1   85.5±0ms
               2   208±0ms
               5   836±0ms
              === ==========

[ 50.00%] ··· constrained_layout.time_subplots_colorbar                                                                                                         ok
[ 50.00%] ··· === =========
               n
              --- ---------
               1   120±0ms
               2   452±0ms
               4   1.35±0s
              === =========

jklymak · 2021-05-14T18:06:48Z

This is inspired by #19892 where we will probably move towards making layout managers more modular....

jklymak · 2021-05-15T15:18:40Z

One note - for constrained_layout, at least, the figure does need to know that it is being used before any colorbars are added so that it can use the non-gridspec colorbars. So, in general, its not possible to strip all layout manager state to draw time. However, this PR is still useful because it allows more layout manager state to be determined at draw time.

jklymak · 2021-06-13T16:38:20Z

I'd like to get this in so #20426 can get in for 3.5. Note this is a relatively straightforward change, despite its apparent length. The logic for creating the layout boxes that comprise constrained_layout are created at draw-time instead of when the figure and gridspecs are created.

jklymak · 2021-06-16T20:15:16Z

lib/matplotlib/_constrained_layout.py



-def _make_layout_margins(fig, renderer, *, w_pad=0, h_pad=0,
+def _make_layout_margins(_layoutgrids, fig, renderer, *, w_pad=0, h_pad=0,


Changes from here and below are just adding the _layoutgrids structure to all the methods....

jklymak · 2021-06-16T20:16:13Z

lib/matplotlib/_constrained_layout.py

+    return _layoutgrids
+
+
+def _make_layoutgrids(fig, _layoutgrids):


this logic was all in figure.py.

jklymak · 2021-06-16T20:16:33Z

lib/matplotlib/_constrained_layout.py

+    return _layoutgrids
+

+def _make_layoutgrids_gs(_layoutgrids, gs):


This logic was all in gridspecs.py

jklymak · 2021-06-16T20:17:41Z

lib/matplotlib/figure.py

        """
        return self._parent.get_constrained_layout_pads(relative=relative)

-    def init_layoutgrid(self):


This was moved to _constrained_layout Not how there are no _layoutgrids attached to the figure any longer...

Do you want to add a changelog note?

Do we need one for private attributes?

jklymak · 2021-06-16T20:18:54Z

lib/matplotlib/gridspec.py

                         width_ratios=width_ratios,
                         height_ratios=height_ratios)

-        # set up layoutgrid for constrained_layout:


All moved to _constrained_layout.py and happens at draw time...

lib/matplotlib/_constrained_layout.py

anntzer · 2021-06-17T19:55:25Z

lib/matplotlib/_constrained_layout.py

+        _layoutgrids[fig] = layoutgrid.LayoutGrid(
+            parent=parentlb,
+            name=(parentlb.name + '.' + 'panellb' +
+                    layoutgrid.seq_id()),


Perhaps you can have a seq_id(<prefix>) function to avoid all these string manipulations everywhere.
(possibly in a separate PR)

Sure, I just moved it into LayoutGrid. The names were just for debug purposes to keep track of what each grid was supposed to be containing.

jklymak · 2021-06-17T20:48:35Z

Benchmark with 30 draws per benchmark:

master:

$ asv dev --bench constrained
· Discovering benchmarks
· Running 2 total benchmarks (1 commits * 1 environments * 2 benchmarks)
[  0.00%] ·· Benchmarking existing-py_Users_jklymak_anaconda3_envs_matplotlibdev_bin_python
[ 25.00%] ··· constrained_layout.time_subplots                                ok
[ 25.00%] ··· === =========
               n           
              --- ---------
               1   868±0ms 
               2   3.27±0s 
               5   17.4±0s 
              === =========

[ 50.00%] ··· constrained_layout.time_subplots_colorbar                       ok
[ 50.00%] ··· === =========
               n           
              --- ---------
               1   2.22±0s 
               2   8.48±0s 
               4   28.9±0s 
              === =========

This PR:

$ asv dev --bench constrained
· Discovering benchmarks
· Running 2 total benchmarks (1 commits * 1 environments * 2 benchmarks)
[  0.00%] ·· Benchmarking existing-py_Users_jklymak_anaconda3_envs_matplotlibdev_bin_python
[ 25.00%] ··· constrained_layout.time_subplots                                ok
[ 25.00%] ··· === =========
               n           
              --- ---------
               1   928±0ms 
               2   3.31±0s 
               5   16.5±0s 
              === =========

[ 50.00%] ··· constrained_layout.time_subplots_colorbar                       ok
[ 50.00%] ··· === =========
               n           
              --- ---------
               1   2.27±0s 
               2   8.98±0s 
               4   28.1±0s 
              === =========

Same benchmark, but `constrained_layout=False`

asv dev --bench constrained
· Discovering benchmarks
· Running 2 total benchmarks (1 commits * 1 environments * 2 benchmarks)
[  0.00%] ·· Benchmarking existing-py_Users_jklymak_anaconda3_envs_matplotlibdev_bin_python
[ 25.00%] ··· constrained_layout.time_subplots                                ok
[ 25.00%] ··· === =========
               n           
              --- ---------
               1   404±0ms 
               2   1.57±0s 
               5   6.59±0s 
              === =========

[ 50.00%] ··· constrained_layout.time_subplots_colorbar                       ok
[ 50.00%] ··· === =========
               n           
              --- ---------
               1   925±0ms 
               2   2.95±0s 
               4   9.16±0s 
              === =========

jklymak · 2021-07-28T17:51:46Z

I'll pop this to "merge with single review". There are no API changes, and its operating on something we still nominally consider experimental.

lib/matplotlib/_constrained_layout.py

QuLogic · 2021-08-06T22:23:18Z

lib/matplotlib/_constrained_layout.py

+    return layoutgrids
+

+def make_layoutgrids(fig, layoutgrids):


All the others take layoutgrids first; should this not do the same also?

This is (usually) called first, so the layoutgrids is only if we are doing the call recursively....

lib/matplotlib/_constrained_layout.py

lib/matplotlib/_layoutgrid.py

lib/matplotlib/_constrained_layout.py

QuLogic · 2021-08-10T21:39:57Z

I think you missed a couple of the alignment comments, but otherwise LGTM.

jklymak · 2021-08-10T23:55:43Z

I think you missed a couple of the alignment comments, but otherwise LGTM.

fixed now, thanks...

jklymak force-pushed the enh-cl-drawtime branch from dc3c027 to 600dd6f Compare May 14, 2021 05:49

jklymak changed the title ~~ENH: Just do constrained layout at draw...~~ ENH: Only do constrained layout at draw... May 14, 2021

jklymak force-pushed the enh-cl-drawtime branch from 600dd6f to bd2cb36 Compare May 14, 2021 14:24

jklymak added the topic: geometry manager LayoutEngine, Constrained layout, Tight layout label May 14, 2021

jklymak force-pushed the enh-cl-drawtime branch from dac4f5a to 949cb33 Compare May 14, 2021 16:54

jklymak added this to the v3.5.0 milestone May 15, 2021

jklymak marked this pull request as ready for review May 15, 2021 15:18

jklymak marked this pull request as draft May 16, 2021 19:26

jklymak mentioned this pull request Jun 13, 2021

ENH: Layout engine #20426

Merged

7 tasks

jklymak marked this pull request as ready for review June 13, 2021 16:37

jklymak added the status: needs review label Jun 13, 2021

jklymak force-pushed the enh-cl-drawtime branch from 7635696 to b036e26 Compare June 16, 2021 19:55

jklymak commented Jun 16, 2021

View reviewed changes

lib/matplotlib/_constrained_layout.py Outdated Show resolved Hide resolved

anntzer reviewed Jun 17, 2021

View reviewed changes

anntzer approved these changes Jun 26, 2021

View reviewed changes

jklymak added the status: merge with single review? label Jul 28, 2021

QuLogic reviewed Aug 6, 2021

View reviewed changes

QuLogic removed status: needs review status: merge with single review? labels Aug 6, 2021

jklymak force-pushed the enh-cl-drawtime branch 2 times, most recently from 5eec6c5 to e61bf9f Compare August 10, 2021 17:56

ENH: just do constrained_layout at draw time

eacda99

jklymak force-pushed the enh-cl-drawtime branch from e61bf9f to eacda99 Compare August 10, 2021 23:55

QuLogic approved these changes Aug 10, 2021

View reviewed changes

QuLogic merged commit a0d2e39 into matplotlib:master Aug 11, 2021

jklymak deleted the enh-cl-drawtime branch August 11, 2021 02:32



		def _make_layout_margins(fig, renderer, *, w_pad=0, h_pad=0,
		def _make_layout_margins(_layoutgrids, fig, renderer, *, w_pad=0, h_pad=0,

		return _layoutgrids


		def _make_layoutgrids(fig, _layoutgrids):

		return _layoutgrids


		def _make_layoutgrids_gs(_layoutgrids, gs):

Uh oh!

Conversation

jklymak commented May 14, 2021 • 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 14, 2021

Benchmarks

master:

This PR:

Uh oh!

jklymak commented May 14, 2021

Uh oh!

jklymak commented May 15, 2021

Uh oh!

jklymak commented Jun 13, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

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!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jklymak commented Jun 17, 2021

Benchmark with 30 draws per benchmark:

master:

This PR:

Same benchmark, but constrained_layout=False

Uh oh!

jklymak commented Jul 28, 2021

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

QuLogic commented Aug 10, 2021

Uh oh!

jklymak commented Aug 10, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

jklymak commented May 14, 2021 •

edited

Loading

jklymak commented Jun 13, 2021 •

edited

Loading

Same benchmark, but `constrained_layout=False`

jklymak commented Aug 10, 2021 •

edited

Loading