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

Rewrite of constrained_layout....#17494

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:new-layout-just-layout
Aug 24, 2020

Conversation

jklymak
Copy link
Member

@jklymakjklymak commentedMay 23, 2020
edited
Loading

PR Summary

This PR rewritesconstrained_layout. There are a couple of motivations. CL largely worked, but was prone to failure. Basically the model was arrange a bunch of axes boxes and have them with a certain minimum margin around themselves, and the axes in the same rows would have their spines aligned. The problem with that approach was that there was no constraint against all the axes being zero width and height, nor that they filled the figure. To get around that there was a weak constraint asking for all the axes to fill the figure, and then the solver would try and make them large rather than small.

However, this failed frequently. One of the tests was marked "flakey" because it would still occasionally collapse to zero width/height. This is obviously not robust nor particularly elegant.

The new approach is API equivalent, but much more robust. The only editable feature are the margins around the axes (there happen to be two of them, one for the decorations and the other for the padding and colorbars). These default to zero width, so the solution is always valid. Of course its still possible for the axes to be zero sized if the decorations are too large, in which case a warning is emitted.

Visually there are some substantial differences. The meaning ofwspace andhsapce was twice as wide as it should have been, at least according to the documentation, so that has been fixed.

More importantly, colorbars have been substantially improved. They now honour theanchor kwarg forcolorbar (closes#14638). This leads to wider colorbars as well, but I'd argue they are more "correct" than they used to be.

In terms of simplicity, the new version now only attaches itself to figures and gridspecs. There are no boxes attached to individual axes etc. Rather each gridspec is divided into columns and rows and all the layout information is contained at the gridspec level. There is still substantially messy bits to do with placing colorbars, largely because left, right, bottom and top placements are all different.

In terms of performance, 1) it actually improves non-CL performance somewhat, and CL performance as well. New version: Constrained: 14.31 s Not: 6.88 s. Old version: Constrained: 15.40 s Not: 7.30 s. So, the new version is actually 6% faster with CL turned off, and about 10% faster with it turned on.

Future advantages: This was motivated by implementing supxlabel and supylabel (#11147). the new layout allows gridspec-wide margins and thus suptitles, colorbars etc.

This will also allow subfigures/subpanels to be implemented recursively (there are stubs in here for this).

PR Checklist

  • Has Pytest style unit tests
  • Code isFlake 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

stonebig and nspies-celsiustx reacted with heart emoji
@jklymakjklymakforce-pushed thenew-layout-just-layout branch from08ee4bd toaaa523cCompareMay 23, 2020 16:35
@jklymakjklymak mentioned this pull requestMay 23, 2020
11 tasks
@jklymakjklymak added the topic: geometry managerLayoutEngine, Constrained layout, Tight layout labelMay 23, 2020
@jklymakjklymak added this to thev3.4.0 milestoneMay 23, 2020
@jklymakjklymak mentioned this pull requestMay 24, 2020
13 tasks
@jklymakjklymakforce-pushed thenew-layout-just-layout branch 2 times, most recently from91f9244 to81b962aCompareMay 25, 2020 02:53
@jklymakjklymak marked this pull request as ready for reviewMay 25, 2020 02:53
@jklymak
Copy link
MemberAuthor

jklymak commentedMay 27, 2020
edited
Loading

An example of a layout that fails master, but works with this PR (from#16603) :

importmatplotlib.pyplotaspltfig=plt.figure(constrained_layout=True)gs=fig.add_gridspec(2,3)ax=dict()ax['A']=fig.add_subplot(gs[0,0:2])ax['B']=fig.add_subplot(gs[1,0:2])ax['C']=fig.add_subplot(gs[:,2])

yields

./testCLGS.py:12: UserWarning: constrained_layout not applied.  At least one axes collapsed to zero width or height.

With this PR, we get the correct result:

CLissue

Thisused to work in v3.0.0, sort of:

CLissue

@jklymakjklymak mentioned this pull requestMay 28, 2020
6 tasks
@anntzer
Copy link
Contributor

See#17712 (comment) for a case where colorbar ticklabels are overlapping with this PR.

return False
return True
"""
General idea:
Copy link
Contributor

Choose a reason for hiding this comment

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

vaguely prefer this to be a comment rather than a string...

Copy link
MemberAuthor

Choose a reason for hiding this comment

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

You mean w/ a bunch of hashes in front?

gs = ss.get_gridspec()
lg = gs._layoutgrid

if hasattr(gs, 'hspace'):
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm not sure why hspace is public on GridSpec but private on GridSpecFromSubplotSpec, but I'd rather just make it public in both (possibly a property on the latter if we want to keep it readonly) rather than having to do this kinds of checks here.

Copy link
MemberAuthor

Choose a reason for hiding this comment

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

Thats fine with me. I don't understand it either. Is there an action item for me here?

hspace=hspace, wspace=wspace)
panel._layoutgrid.parent.edit_outer_margin_mins(margins, ss)

# for ax in [a for a in fig._localaxes if hasattr(a, 'get_subplotspec')]:
Copy link
Contributor

Choose a reason for hiding this comment

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

remove?

Copy link
MemberAuthor

Choose a reason for hiding this comment

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

Well, I'm hoping to get a version in that uses_localaxes for 3.4, so this was just bread crumbing....

often just set to 1 for an equal grid.

Subplotspecs that are derived from this gridspec can contain either a
``SubPanel``, a ``GridSpecFromSubplotSpec``, or an axes. The ``SubPanel`` and
Copy link
Contributor

Choose a reason for hiding this comment

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

transpanel probably needs to be added to the transforms tutorial table, if it's intended to be public.

Copy link
MemberAuthor

Choose a reason for hiding this comment

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

Again, this is meant to be public soon - its only used here for now and is the same as transFigure. This is all for the sub panel feature, but its too much to have both in the same PR....

@@ -568,15 +569,10 @@ def __init__(self, fig, rect,
rcParams['ytick.major.right']),
which='major')

self._layoutbox = None
Copy link
MemberAuthor

Choose a reason for hiding this comment

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

Note axes no longer carry state around...

@QuLogic
Copy link
Member

I meant to add an overall comment: These are mostly stylistic, but I did have a few clarifying questions.

@jklymakjklymakforce-pushed thenew-layout-just-layout branch fromfb25a32 toa5a6e0aCompareAugust 17, 2020 22:34
Copy link
Member

@tacaswelltacaswell left a comment

Choose a reason for hiding this comment

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

From discussion on call this :

  • fixes a number of pathological failure modes
  • does not change the public API
  • removes state from the Axes objects

This may however change the exact layout of some figures, but given that this was marked as provisional that is ok.

@jklymakjklymakforce-pushed thenew-layout-just-layout branch 2 times, most recently froma21c97c to81ba29cCompareAugust 18, 2020 17:25
@jklymakjklymak marked this pull request as draftAugust 18, 2020 17:49
@jklymak
Copy link
MemberAuthor

Just sticking in draft until I have time to work on@QuLogic helpful suggestions....

@jklymakjklymakforce-pushed thenew-layout-just-layout branch from2206768 to44af4aaCompareAugust 19, 2020 02:02
@jklymakjklymak marked this pull request as ready for reviewAugust 19, 2020 02:24
Copy link
Member

@QuLogicQuLogic left a comment

Choose a reason for hiding this comment

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

Nothing too significant left, mostly just typos.

@jklymakjklymakforce-pushed thenew-layout-just-layout branch 4 times, most recently fromcb2c2e9 to11971ccCompareAugust 19, 2020 22:05
Copy link
Member

@QuLogicQuLogic left a comment

Choose a reason for hiding this comment

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

OK, can squash.

Apply suggestions from code reviewCo-authored-by: Elliott Sales de Andrade <quantum.analyst@gmail.com>
@jklymakjklymakforce-pushed thenew-layout-just-layout branch from11971cc to8db28aaCompareAugust 20, 2020 05:56
@jklymak
Copy link
MemberAuthor

I'll self merge this, unless someone else wants to take the plunge!

@QuLogicQuLogic merged commite78aee9 intomatplotlib:masterAug 24, 2020
@jklymak
Copy link
MemberAuthor

Thanks a lot for your reviews@anntzer@QuLogic and@tacaswell

@anntzer
Copy link
Contributor

@jklymak Just to say that I had some code around using CL to place a few dozen axes on a figure; the new version is at least 10x faster than the old one (<1s vs ~10s). The improvement is really nice :-)

@jklymak
Copy link
MemberAuthor

Thanks@anntzer - glad it is working. You know how it is - the second time you write something, its a bit easier!

anntzer and yoda-vid reacted with thumbs up emoji

Sign up for freeto join this conversation on GitHub. Already have an account?Sign in to comment
Reviewers

@anntzeranntzeranntzer left review comments

@tacaswelltacaswelltacaswell approved these changes

@QuLogicQuLogicQuLogic approved these changes

Assignees
No one assigned
Labels
topic: geometry managerLayoutEngine, Constrained layout, Tight layout
Projects
None yet
Milestone
v3.4.0
4 participants
@jklymak@anntzer@QuLogic@tacaswell

[8]ページ先頭

©2009-2025 Movatter.jp