Skip to content
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

ENH: Only do constrained layout at draw... #20229

Merged
merged 1 commit into from Aug 11, 2021

Conversation

@jklymak
Copy link
Contributor

@jklymak jklymak commented May 14, 2021

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.

  1. No extra state to worry about if the figure is serialized (kiwi solver objects don't serialize).
  2. constrained_layout can respond to changing gridspec specs (like width ratios) because all the state is decided at draw time.
  3. 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 jklymak force-pushed the jklymak:enh-cl-drawtime branch from dc3c027 to 600dd6f May 14, 2021
@jklymak jklymak changed the title ENH: Just do constrained layout at draw... ENH: Only do constrained layout at draw... May 14, 2021
@jklymak jklymak force-pushed the jklymak:enh-cl-drawtime branch from 600dd6f to bd2cb36 May 14, 2021
@jklymak
Copy link
Contributor Author

@jklymak jklymak commented May 14, 2021

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 jklymak force-pushed the jklymak:enh-cl-drawtime branch from dac4f5a to 949cb33 May 14, 2021
@jklymak
Copy link
Contributor Author

@jklymak jklymak commented May 14, 2021

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

@jklymak
Copy link
Contributor Author

@jklymak jklymak commented May 15, 2021

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 jklymak added this to the v3.5.0 milestone May 15, 2021
@jklymak jklymak marked this pull request as ready for review May 15, 2021
@jklymak jklymak marked this pull request as draft May 16, 2021
@jklymak jklymak mentioned this pull request Jun 13, 2021
7 tasks
@jklymak jklymak marked this pull request as ready for review Jun 13, 2021
@jklymak
Copy link
Contributor Author

@jklymak jklymak commented Jun 13, 2021

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 jklymak added this to In progress in Layout engine via automation Jun 13, 2021
@jklymak jklymak force-pushed the jklymak:enh-cl-drawtime branch from 7635696 to b036e26 Jun 16, 2021
@@ -188,7 +283,7 @@ def _get_margin_from_padding(obj, *, w_pad=0, h_pad=0,
return margin


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,

This comment has been minimized.

@jklymak

jklymak Jun 16, 2021
Author Contributor

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

return _layoutgrids


def _make_layoutgrids(fig, _layoutgrids):

This comment has been minimized.

@jklymak

jklymak Jun 16, 2021
Author Contributor

this logic was all in figure.py.


def _make_layoutgrids_gs(_layoutgrids, gs):

This comment has been minimized.

@jklymak

jklymak Jun 16, 2021
Author Contributor

This logic was all in gridspecs.py

@@ -2065,21 +2058,6 @@ def get_constrained_layout_pads(self, relative=False):
"""
return self._parent.get_constrained_layout_pads(relative=relative)

def init_layoutgrid(self):

This comment has been minimized.

@jklymak

jklymak Jun 16, 2021
Author Contributor

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

This comment has been minimized.

@anntzer

anntzer Jun 17, 2021
Contributor

Do you want to add a changelog note?

This comment has been minimized.

@jklymak

jklymak Jul 5, 2021
Author Contributor

Do we need one for private attributes?

This comment has been minimized.

@timhoffm

timhoffm Jul 6, 2021
Member

Nope.

@@ -387,25 +385,8 @@ def __init__(self, nrows, ncols, figure=None,
width_ratios=width_ratios,
height_ratios=height_ratios)

# set up layoutgrid for constrained_layout:

This comment has been minimized.

@jklymak

jklymak Jun 16, 2021
Author Contributor

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

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

This comment has been minimized.

@anntzer

anntzer Jun 17, 2021
Contributor

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

This comment has been minimized.

@jklymak

jklymak Jun 17, 2021
Author Contributor

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
Copy link
Contributor Author

@jklymak jklymak commented Jun 17, 2021

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
Copy link
Contributor Author

@jklymak jklymak commented Jul 28, 2021

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 Show resolved Hide resolved
lib/matplotlib/_constrained_layout.py Outdated Show resolved Hide resolved

def make_layoutgrids(fig, layoutgrids):

This comment has been minimized.

@QuLogic

QuLogic Aug 6, 2021
Member

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

This comment has been minimized.

@jklymak

jklymak Aug 10, 2021
Author Contributor

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

lib/matplotlib/_constrained_layout.py Outdated Show resolved Hide resolved
lib/matplotlib/_constrained_layout.py Outdated Show resolved Hide resolved
lib/matplotlib/_constrained_layout.py Outdated Show resolved Hide resolved
lib/matplotlib/_constrained_layout.py Outdated Show resolved Hide resolved
lib/matplotlib/_constrained_layout.py Outdated Show resolved Hide resolved
lib/matplotlib/_layoutgrid.py Outdated Show resolved Hide resolved
lib/matplotlib/_constrained_layout.py Outdated Show resolved Hide resolved
@jklymak jklymak force-pushed the jklymak:enh-cl-drawtime branch 2 times, most recently from 5eec6c5 to e61bf9f Aug 10, 2021
@QuLogic
Copy link
Member

@QuLogic QuLogic commented Aug 10, 2021

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

@jklymak jklymak force-pushed the jklymak:enh-cl-drawtime branch from e61bf9f to eacda99 Aug 10, 2021
@jklymak
Copy link
Contributor Author

@jklymak jklymak commented Aug 10, 2021

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

fixed now, thanks...

@QuLogic QuLogic merged commit a0d2e39 into matplotlib:master Aug 11, 2021
27 checks passed
27 checks passed
@github-actions
flake8
Details
@github-actions
greeting
Details
@github-actions
Python 3.7 on ubuntu-18.04 (Minimum Versions)
Details
@github-actions
Python 3.7 on ubuntu-18.04
Details
@github-actions
Python 3.8 on ubuntu-18.04
Details
@github-actions
Python 3.9 on ubuntu-20.04
Details
@github-actions
Python 3.8 on macos-latest
Details
@github-actions
eslint
Details
@lgtm-com
LGTM analysis: JavaScript No code changes detected
Details
@github-actions
Check the rendered docs here! Link to 0/doc/build/html/index.html
Details
@lgtm-com
LGTM analysis: Python 2 fixed alerts
Details
ci/circleci: docs-python38 Your tests passed on CircleCI!
Details
@codecov
codecov/patch 88.39% of diff hit (target 50.00%)
Details
@codecov
codecov/project/library 80.52% (target 50.00%)
Details
@codecov
codecov/project/tests 98.84% (+0.00%) compared to b61e76a
Details
@appveyor
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
@azure-pipelines
matplotlib.matplotlib Build #20210810.18 succeeded
Details
@azure-pipelines
matplotlib.matplotlib (Check Skip) Check Skip succeeded
Details
@azure-pipelines
matplotlib.matplotlib (Main Pytest Linux_py37) Main Pytest Linux_py37 succeeded
Details
@azure-pipelines
matplotlib.matplotlib (Main Pytest Linux_py38) Main Pytest Linux_py38 succeeded
Details
@azure-pipelines
matplotlib.matplotlib (Main Pytest Linux_py39) Main Pytest Linux_py39 succeeded
Details
@azure-pipelines
matplotlib.matplotlib (Main Pytest Windows_py37) Main Pytest Windows_py37 succeeded
Details
@azure-pipelines
matplotlib.matplotlib (Main Pytest Windows_py38) Main Pytest Windows_py38 succeeded
Details
@azure-pipelines
matplotlib.matplotlib (Main Pytest Windows_py39) Main Pytest Windows_py39 succeeded
Details
@azure-pipelines
matplotlib.matplotlib (Main Pytest macOS_py37) Main Pytest macOS_py37 succeeded
Details
@azure-pipelines
matplotlib.matplotlib (Main Pytest macOS_py38) Main Pytest macOS_py38 succeeded
Details
@azure-pipelines
matplotlib.matplotlib (Main Pytest macOS_py39) Main Pytest macOS_py39 succeeded
Details
Layout engine automation moved this from Needs review to Done Aug 11, 2021
v3.5 Milestone automation moved this from Medium Priority to Done Aug 11, 2021
@jklymak jklymak deleted the jklymak:enh-cl-drawtime branch Aug 11, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Linked issues

Successfully merging this pull request may close these issues.

None yet

4 participants