Movatterモバイル変換

[0]ホーム
URL:

Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

 Sign up
Appearance settings

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

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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.

Already on GitHub?Sign in to your account

Merged
QuLogic merged 1 commit intomatplotlib:masterfromjklymak:enh-cl-drawtime
Aug 11, 2021

Conversation

jklymak
Copy link
Member

@jklymakjklymak commentedMay 14, 2021•
edited
Loading

PR Summary

This PR moves all of the state for constrained_layout out offigure andgridspec 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 (andpytest passes).
  • IsFlake 8 compliant (runflake8 on changed files to check).
  • New features are documented, with examples if plot related.
  • Documentation is sphinx and numpydoc compliant (the docs shouldbuild without error).
  • Conforms to Matplotlib style conventions (installflake8-docstrings and runflake8 --docstring-convention=all).
  • New features have an entry indoc/users/next_whats_new/ (follow instructions in README.rst there).
  • API changes documented indoc/api/next_api_changes/ (follow instructions in README.rst there).

@jklymakjklymak changed the titleENH: Just do constrained layout at draw...ENH: Only do constrained layout at draw...May 14, 2021
@jklymakjklymak added the topic: geometry managerLayoutEngine, Constrained layout, Tight layout labelMay 14, 2021
@jklymak
Copy link
MemberAuthor

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
Copy link
MemberAuthor

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

@jklymak
Copy link
MemberAuthor

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.

@jklymakjklymak added this to thev3.5.0 milestoneMay 15, 2021
@jklymakjklymak marked this pull request as ready for reviewMay 15, 2021 15:18
@jklymakjklymak marked this pull request as draftMay 16, 2021 19:26
@jklymakjklymak mentioned this pull requestJun 13, 2021
7 tasks
@jklymakjklymak marked this pull request as ready for reviewJune 13, 2021 16:37
@jklymak
Copy link
MemberAuthor

jklymak commentedJun 13, 2021•
edited
Loading

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,
Copy link
MemberAuthor

Choose a reason for hiding this comment

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

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

return _layoutgrids


def _make_layoutgrids(fig, _layoutgrids):
Copy link
MemberAuthor

Choose a reason for hiding this comment

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

this logic was all infigure.py.


def _make_layoutgrids_gs(_layoutgrids, gs):
Copy link
MemberAuthor

Choose a reason for hiding this comment

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

This logic was all ingridspecs.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):
Copy link
MemberAuthor

Choose a reason for hiding this comment

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

This was moved to_constrained_layout Not how there are no_layoutgrids attached to the figure any longer...

Copy link
Contributor

Choose a reason for hiding this comment

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

Do you want to add a changelog note?

Copy link
MemberAuthor

Choose a reason for hiding this comment

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

Do we need one for private attributes?

Copy link
Member

Choose a reason for hiding this comment

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

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:
Copy link
MemberAuthor

Choose a reason for hiding this comment

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

All moved to_constrained_layout.py and happens at draw time...

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

Choose a reason for hiding this comment

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

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

Copy link
MemberAuthor

Choose a reason for hiding this comment

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

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
MemberAuthor

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, butconstrained_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
MemberAuthor

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):
Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
MemberAuthor

Choose a reason for hiding this comment

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

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

@jklymakjklymakforce-pushed theenh-cl-drawtime branch 2 times, most recently from5eec6c5 toe61bf9fCompareAugust 10, 2021 17:56
@QuLogic
Copy link
Member

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

@jklymak
Copy link
MemberAuthor

jklymak commentedAug 10, 2021•
edited
Loading

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

fixed now, thanks...

@QuLogicQuLogic merged commita0d2e39 intomatplotlib:masterAug 11, 2021
@jklymakjklymak deleted the enh-cl-drawtime branchAugust 11, 2021 02:32
Sign up for freeto join this conversation on GitHub. Already have an account?Sign in to comment
Reviewers

@timhoffmtimhoffmtimhoffm left review comments

@QuLogicQuLogicQuLogic approved these changes

@anntzeranntzeranntzer approved these changes

Assignees
No one assigned
Labels
topic: geometry managerLayoutEngine, Constrained layout, Tight layout
Projects
None yet
Milestone
v3.5.0
Development

Successfully merging this pull request may close these issues.

4 participants
@jklymak@QuLogic@anntzer@timhoffm
[8]ページ先頭
©2009-2025 Movatter.jp