ENH: Only do constrained layout at draw... #20229
Conversation
Benchmarkshttps://github.com/matplotlib/mpl-bench
master:
This PR:
|
This is inspired by #19892 where we will probably move towards making layout managers more modular.... |
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. |
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. |
@@ -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, |
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): |
|
||
def _make_layoutgrids_gs(_layoutgrids, gs): |
@@ -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): |
jklymak
Jun 16, 2021
Author
Contributor
This was moved to _constrained_layout
Not how there are no _layoutgrids
attached to the figure any longer...
@@ -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: |
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()), |
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)
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.
Benchmark with 30 draws per benchmark:master:
This PR:
Same benchmark, but
|
I'll pop this to "merge with single review". There are no API changes, and its operating on something we still nominally consider experimental. |
|
||
def make_layoutgrids(fig, layoutgrids): |
jklymak
Aug 10, 2021
Author
Contributor
This is (usually) called first, so the layoutgrids
is only if we are doing the call recursively....
5eec6c5
to
e61bf9f
I think you missed a couple of the alignment comments, but otherwise LGTM. |
fixed now, thanks... |
PR Summary
This PR moves all of the state for constrained_layout out of
figure
andgridspec
except for the kwargs in figure. There are a few clear advantages.This may have a minor speed hit because all the layout boxes are made each draw, but I doubt its very significant
PR Checklist
pytest
passes).flake8
on changed files to check).flake8-docstrings
and runflake8 --docstring-convention=all
).doc/users/next_whats_new/
(follow instructions in README.rst there).doc/api/next_api_changes/
(follow instructions in README.rst there).