Rewrite of constrained_layout.... #17494
Conversation
08ee4bd to
aaa523c
Compare
91f9244 to
81b962a
Compare
|
An example of a layout that fails master, but works with this PR (from #16603) : import matplotlib.pyplot as plt
fig = 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 With this PR, we get the correct result:
This used to work in v3.0.0, sort of:
|
81b962a to
4e1ddc6
Compare
|
See #17712 (comment) for a case where colorbar ticklabels are overlapping with this PR. |
| return False | ||
| return True | ||
| """ | ||
| General idea: |
There was a problem hiding this comment.
vaguely prefer this to be a comment rather than a string...
There was a problem hiding this comment.
You mean w/ a bunch of hashes in front?
| gs = ss.get_gridspec() | ||
| lg = gs._layoutgrid | ||
|
|
||
| if hasattr(gs, 'hspace'): |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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')]: |
There was a problem hiding this comment.
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 |
There was a problem hiding this comment.
transpanel probably needs to be added to the transforms tutorial table, if it's intended to be public.
There was a problem hiding this comment.
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 | |||
There was a problem hiding this comment.
Note axes no longer carry state around...
|
I meant to add an overall comment: These are mostly stylistic, but I did have a few clarifying questions. |
fb25a32 to
a5a6e0a
Compare
a21c97c to
81ba29c
Compare
|
Just sticking in draft until I have time to work on @QuLogic helpful suggestions.... |
2206768 to
44af4aa
Compare
cb2c2e9 to
11971cc
Compare
Apply suggestions from code review Co-authored-by: Elliott Sales de Andrade <quantum.analyst@gmail.com>
11971cc to
8db28aa
Compare
|
I'll self merge this, unless someone else wants to take the plunge! |
|
Thanks a lot for your reviews @anntzer @QuLogic and @tacaswell |
|
@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 :-) |
|
Thanks @anntzer - glad it is working. You know how it is - the second time you write something, its a bit easier! |
PR Summary
This PR rewrites
constrained_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 of
wspaceandhsapcewas 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 the
anchorkwarg 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