FIX: constrained_layout and repeated calls to suptitle #11035

jklymak · 2018-04-12T16:10:12Z

PR Summary

As pointed out by @afvincent on gitter, repeated calls to suptitle using constrained_layout=True were making room for each call. This fixes the logic to only create the layout box if the subtitle is new.

Note that the change is just indenting a block...

import matplotlib.pyplot as plt

fig, axs = plt.subplots(1, 2, num="issue_suptitle", constrained_layout=True)

for idx in range(10):
    fig.suptitle(f"Suptitle #{idx}")
plt.show()

Before

Now

PR Checklist

Code is PEP 8 compliant

jklymak · 2018-04-12T16:11:05Z

self-assigning for backport, but this is certainly minor enough that I don't think its a big deal if its not backported.

afvincent · 2018-04-12T18:24:12Z

lib/matplotlib/figure.py

+                        w_pad, h_pad, wspace, hspace =  \
+                                self.get_constrained_layout_pads(
+                                        relative=True)
+                        layoutbox.vstack([self._suptitle._layoutbox, child],


@jklymak Naive question: does the use of layout.vstack mean that one assume that the suptitle will always be above all the other elements?

jklymak · 2018-04-12T18:26:38Z

Yes. Is it possible to place suptitles manually?

afvincent · 2018-04-12T18:36:18Z

Apparently, according to the docstring, one can use an arbitrary position for suptitle

import matplotlib.pyplot as plt
plt.ion()
fig, ax = plt.subplots(constrained_layout=True)
fig.suptitle("This is a lower right suptitle", x=0.80, y=0.05)

Not sure what should be the exact behavior though with respect the constrained layout :/

Edit: FWIW, it looks like some space is saved at the top of the figure, even though the suptitle ends up in the lower right corner. (One can increase the font size of the suptitle to better see this behavior.)

jklymak · 2018-04-12T19:08:21Z

OK, new push: if user places subtitle position manually, it doesn't get to be part of constrained_layout.

jklymak · 2018-04-18T17:04:53Z

Ping @afvincent does this fix the issue for you?

afvincent · 2018-04-24T02:48:22Z

It seems that this needs a rebase (sorry for delaying 🐑)

Besides, is it expected that running twice (in an IPython shell, FWIW) the following snippet

import matplotlib.pyplot as plt

fig, axs = plt.subplots(1, 2, num="issue_suptitle", constrained_layout=True,
                        clear=True)  # <- the culprit (as usual)

for idx in range(10):
    fig.suptitle(f"Suptitle #{idx}")
plt.show(block=False)

throws

---------------------------------------------------------------------------
AttributeError                            Traceback (most recent call last)
<ipython-input-3-e6132691fc2f> in <module>()
      1 import matplotlib.pyplot as plt
      2 
----> 3 fig, axs = plt.subplots(1, 2, num="issue_suptitle", clear=True, constrained_layout=True)
      4 
      5 for idx in range(10):

~/FOSS/matplotlib/lib/matplotlib/pyplot.py in subplots(nrows, ncols, sharex, sharey, squeeze, subplot_kw, gridspec_kw, **fig_kw)
   1102     subplot
   1103     """
-> 1104     fig = figure(**fig_kw)
   1105     axs = fig.subplots(nrows=nrows, ncols=ncols, sharex=sharex, sharey=sharey,
   1106                        squeeze=squeeze, subplot_kw=subplot_kw,

~/FOSS/matplotlib/lib/matplotlib/pyplot.py in figure(num, figsize, dpi, facecolor, edgecolor, frameon, FigureClass, clear, **kwargs)
    549 
    550     if clear:
--> 551         figManager.canvas.figure.clear()
    552 
    553     return figManager.canvas.figure

~/FOSS/matplotlib/lib/matplotlib/figure.py in clear(self, keep_observers)
   1438         Clear the figure -- synonym for :meth:`clf`.
   1439         """
-> 1440         self.clf(keep_observers=keep_observers)
   1441 
   1442     @allow_rasterization

~/FOSS/matplotlib/lib/matplotlib/figure.py in clf(self, keep_observers)
   1431         self._suptitle = None
   1432         if self.get_constrained_layout():
-> 1433             layoutbox.nonetree(self._layoutbox)
   1434         self.stale = True
   1435 

~/FOSS/matplotlib/lib/matplotlib/_layoutbox.py in nonetree(lb)
    671             # Clear the solver.  Hopefully this garbage collects.
    672             lb.solver.reset()
--> 673             nonechildren(lb)
    674         else:
    675             nonetree(lb.parent)

~/FOSS/matplotlib/lib/matplotlib/_layoutbox.py in nonechildren(lb)
    678 def nonechildren(lb):
    679     for child in lb.children:
--> 680         nonechildren(child)
    681     lb.artist._layoutbox = None
    682     lb = None

~/FOSS/matplotlib/lib/matplotlib/_layoutbox.py in nonechildren(lb)
    679     for child in lb.children:
    680         nonechildren(child)
--> 681     lb.artist._layoutbox = None
    682     lb = None
    683 

AttributeError: 'NoneType' object has no attribute '_layoutbox'

if one does not close the first figure window?

(Python 3.6, Matplotlib 2.2.2.post760+ga0e4c044a, Fedora 27)

jklymak · 2018-04-24T03:31:29Z

@afvincent its not expected that anyone would use clear=True 😉 Seriously what does that do?

afvincent · 2018-04-24T03:48:50Z

I have like a feeling of déjà-vu ^^ (but I cannot find the related issue and PR). From the docstring of plt.figure:

clear : bool, optional, default: False
    If True and the figure already exists, then it is cleared.

Personally I use it quite a lot 🐑 since I switched from:

figlabel = "myfigure"
plt.close(figlabel)
fig, ax = plt.subplots(num=figlabel)

to

fig, ax = plt.subplots(num="myfigure", clear=True)  # + constrained_layout=True ^^

which helps me to avoid opening dozens of figures when I am interactively writing plotting scripts through a highly evolved process known as “trial and error”.

jklymak · 2018-04-24T03:59:39Z

OK, rebased. I'll make a different PR for the other issue...

jklymak · 2018-04-24T04:12:28Z

@afvincent, can you open an issue with a reproducible example of the above behaviour? The following works fine for me on master:

import matplotlib.pyplot as plt

fig, ax = plt.subplots(constrained_layout=True, num='boo')
ax.plot(range(10))
plt.show()

fig, ax = plt.subplots(constrained_layout=True, num='boo', clear=True)
ax.plot(range(30))
plt.show()

EDIT: Ooops, Ok I can reproduce if I do plt.show(block=False) in an ipython session. I dunno, might be a tough fix/won't fix. It needs interactive mode, and two non-standard kwargs to figure, and one to show.... I think trial and error is great, but I just close and re-open the window on each trial (well, what I really do is use a jupyter notebook).

afvincent · 2018-04-24T04:27:48Z

You have to use a suptitle on the first figure, see #11115 ;)

jklymak · 2018-04-24T04:29:16Z

Oh, really OK, so maybe it is this PR...

jklymak · 2018-04-24T04:40:56Z

OK, that wasn't as bad as I feared - I wasn't setting the artist for the _suptitle.layoutbox.

afvincent · 2018-04-24T04:42:16Z

lib/matplotlib/figure.py

+        manual_position = ('x' in kwargs or 'y' in kwargs)
+
+        x = kwargs.pop('x', 0.5)
+        y = kwargs.pop('y', 0.98)


Do these two lines really work? I may be naive, but neither 'x' nor 'y' belong to **kwargs, do they not?

Blame @anntzer 😉 The new signature is def suptitle(self, t, *, x=.5, y=.98, **kwargs): The little star in there means everything past the star is a kwarg; x and y are explicit kwargs...

Oh, actually, I think see what happened. @anntzer put the new signature in, and I put the pop back in. OTOH, now I'm confused if manual position ever comes back false now....

EDIT: no, it works... Thanks for catching this...

Ok, neat ^^, but even if 'x' or 'y' are keyword-only arguments, do they belong to the special dictionary **kwargs?

Edit: yes, I think that the popping just makes it to always default to (0.5, 0.98) I think

ping @anntzer is this OK, or is there a better way to check if x or y have been set?

No, you have to use kwargs-popping in that case.

I'm a bit confused though (haven't followed the issue at all): if x and y are not set, are they always going to be 0.5 and 0.98, or is it that the constrained layout manager will move them around? If the latter, then perhaps the default should just be x=y=None, as 0.5/0.98 don't really mean anything? Although I guess they are still meaningful if the constrained layout manager is not in use...

If x and y are not set, then they start life as 0.5 and 0.98. If constrained layout is called, they will move. If the user does set x or y then constrained_layout ignites the suptitle

afvincent · 2018-04-24T04:45:07Z

@jklymak If I am correct, a correct fix should pass the following test, should it not ?

def test_constrained_layout21():
    '#11035: repeated calls to suptitle should not alter the layout'
    fig, ax = plt.subplots(constrained_layout=True)

    fig.suptitle(f"Suptitle #0")
    fig.canvas.draw()
    extents0 = np.copy(ax.get_position().extents)

    fig.suptitle(f"Suptitle #1")
    fig.canvas.draw()
    extents1 = np.copy(ax.get_position().extents)

    np.testing.assert_allclose(extents0, extents1)


def test_constrained_layout22():
    '#11035: suptitle should not be include in CL if manually positioned'
    fig, ax = plt.subplots(constrained_layout=True)

    fig.canvas.draw()
    extents0 = np.copy(ax.get_position().extents)

    fig.suptitle(f"Suptitle", y=0.5)
    fig.canvas.draw()
    extents1 = np.copy(ax.get_position().extents)

    np.testing.assert_allclose(extents0, extents1)


def test_constrained_layout23():
    '''
    Comment in #11035: suptitle used to cause an exception when
    reusing a figure w/ CL with ``clear=True``.
    '''

    for i in range(2):
        fig, ax = plt.subplots(num="123", constrained_layout=True, clear=True)
        fig.suptitle(f"Suptitle {i}".format(i))

afvincent · 2018-04-24T04:48:29Z

My previous comment about popping the 'x' or 'y' kwargs is due to the fact that test_constrained_layout22 does not seem to be put at y=0.5 with a recent version of the current PR.

BTW, the tests are free to steal if you want ^^: I was planning to push them against your branch. But you are definitively too reactive for me to follow the latest rebase 🐑 .

afvincent · 2018-04-24T04:57:37Z

lib/matplotlib/tests/test_constrainedlayout.py

+
+    fig.suptitle(f"Suptitle", y=0.5)
+    fig.canvas.draw()
+    fig.canvas.draw()


Why double draw?

afvincent · 2018-04-24T04:57:44Z

lib/matplotlib/tests/test_constrainedlayout.py

+    fig, ax = plt.subplots(constrained_layout=True)
+
+    fig.canvas.draw()
+    fig.canvas.draw()


Why double draw?

oops... was testing...

jklymak · 2018-04-24T04:58:14Z

OK @anntzer:

Before, for this PR I was checking if kwargs x or y we used when calling suptitle. In a recent PR you changed the signature to def suptitle(self, t, *, x=0.5, y=0.85, **kwargs): But then the test:
manual_position = ('x' in kwargs or 'y' in kwargs) always returns False. Is there a way of testing if the user has passed a non-default value to x or y other than testing the values themselves?

afvincent

LGTM.

Conditional to the CI passing (but I wrote parts of the new tests related to this issue so it may be a bit of a borderline self-approval :/)

afvincent · 2018-04-24T05:11:43Z

lib/matplotlib/figure.py

+                                                child],
+                                                padding=h_pad*2.,
+                                                strength='required')
+                else:


Purely a matter of personal taste and not a blocker at all, but I would have rather written

if manual_position: self._suptitle._layoutbox = None else: figlb = ...

which I find a bit easier to read as in the other case, the long indented blocks may make it easy to misread the level indenting.

Agreed that it seems more legible to move the manual_position case out first.

afvincent · 2018-04-24T05:26:41Z

🐑 for the f-strings :/...

jklymak · 2018-04-28T16:49:19Z

ping @afvincent

afvincent

Two very minor comments if we really care about the docstrings of tests (apologies @jklymak the test drafts that I posted were a bit in a rough state 🐑)... Apart from this optional nitpicking, LGTM :).

I am a bit reluctant to hit the green button myself because I wrote the sketch of the tests and those were not here when @tacaswell gave his approval IIRC. I would not want to make it look like borderline self-merging.

Last question, should this actually be backported into 2.2.3? (Not sure that I followed the last updates on where we decided to put the line)

afvincent · 2018-05-02T23:55:25Z

lib/matplotlib/tests/test_constrainedlayout.py

+
+
+def test_constrained_layout22():
+    '#11035: suptitle should not be include in CL if manually positioned'


included?
(this one comes from me, sorry :/)

I don't think we need to proofread the test comment strings! 😆

I should have written "... very (very) minor comments..." in my review ^^.

Thanks for your help on this! Great that someone (other than me) is using CL. BTW< don't be shy about making more bug reports if you have them.

afvincent · 2018-05-02T23:58:10Z

lib/matplotlib/tests/test_constrainedlayout.py

+def test_constrained_layout23():
+    '''
+    Comment in #11035: suptitle used to cause an exception when
+    reusing a figure w/ CL with ``clear=True``.


Being there, a better phrasing might be "... a figure with CL and clear=True".

jklymak · 2018-05-03T00:38:47Z

2.2.3 backports should be benign; This is all self contained, so I think benign.

anntzer · 2018-05-06T21:27:53Z

lib/matplotlib/figure.py

+                                                artist=self._suptitle,
+                                                name=figlb.name+'.suptitle')
+                    for child in figlb.children:
+                        if not (child == self._suptitle._layoutbox):


if child != self._suptitle._layoutbox
or even
if child is not self._suptitle._layoutbox
given that identity should be the same as equality for layoutboxes?

anntzer · 2018-05-06T21:29:00Z

lib/matplotlib/figure.py

+                                    self.get_constrained_layout_pads(
+                                            relative=True)
+                            layoutbox.vstack([self._suptitle._layoutbox,
+                                                child],


slightly weird indentation

Oh yeah, I rearranged this a bit so the indentation wasn't as bad.

OTOH, I was purposely stacking children on top of each other for v-stack as a cisual clue they are being vertically stacked. So I kept that in.

FIX: make suptitle be in CL only if placed automatically FIX: suptitle needed an artist as well FIX: revert change to signature so we can test if kwargs used fix CL tests

jklymak · 2018-05-07T16:18:59Z

Comments addressed; squashed; rebased

lumberbot-app · 2018-05-07T16:43:25Z

There seem to be a conflict, please backport manually

jklymak · 2018-05-07T17:13:43Z

Manual backport at #11184

Manual backport #11035

jklymak added the topic: geometry manager LayoutEngine, Constrained layout, Tight layout label Apr 12, 2018

jklymak added this to the v2.2.3 milestone Apr 12, 2018

afvincent reviewed Apr 12, 2018

View reviewed changes

jklymak force-pushed the fix-suptitle-CL branch from 0be9701 to a0e4c04 Compare April 12, 2018 19:48

tacaswell approved these changes Apr 15, 2018

View reviewed changes

jklymak force-pushed the fix-suptitle-CL branch from a0e4c04 to b9da1f6 Compare April 24, 2018 03:59

afvincent mentioned this pull request Apr 24, 2018

Reusing cleared figure with suptitle and constrained layout raises an exception #11115

Closed

afvincent reviewed Apr 24, 2018

View reviewed changes

jklymak force-pushed the fix-suptitle-CL branch from 55f22d9 to 0e47dab Compare April 24, 2018 04:59

afvincent approved these changes Apr 24, 2018

View reviewed changes

jklymak force-pushed the fix-suptitle-CL branch 2 times, most recently from b26e978 to 0b69731 Compare April 24, 2018 13:51

afvincent approved these changes May 3, 2018

View reviewed changes

anntzer reviewed May 6, 2018

View reviewed changes

FIX: constrained_layout and repeated calls to suptitle

3ef6944

FIX: make suptitle be in CL only if placed automatically FIX: suptitle needed an artist as well FIX: revert change to signature so we can test if kwargs used fix CL tests

jklymak force-pushed the fix-suptitle-CL branch from 0b69731 to 3ef6944 Compare May 7, 2018 16:17

anntzer merged commit 922c950 into matplotlib:master May 7, 2018

lumberbot-app bot added the status: needs manual backport label May 7, 2018

jklymak mentioned this pull request May 7, 2018

Manual backport #11035 #11184

Merged

6 tasks

jklymak removed the status: needs manual backport label May 7, 2018

jklymak added a commit that referenced this pull request May 7, 2018

Merge pull request #11184 from jklymak/v2.2.x

a3ee438

Manual backport #11035

jklymak deleted the fix-suptitle-CL branch May 7, 2018 18:56



		def test_constrained_layout22():
		'#11035: suptitle should not be include in CL if manually positioned'

FIX: constrained_layout and repeated calls to suptitle #11035

FIX: constrained_layout and repeated calls to suptitle #11035

Conversation

jklymak commented Apr 12, 2018

PR Summary

Before

Now

PR Checklist

jklymak commented Apr 12, 2018

Choose a reason for hiding this comment

jklymak commented Apr 12, 2018

afvincent commented Apr 12, 2018 • edited

jklymak commented Apr 12, 2018

jklymak commented Apr 18, 2018

afvincent commented Apr 24, 2018 • edited by jklymak

jklymak commented Apr 24, 2018

afvincent commented Apr 24, 2018

jklymak commented Apr 24, 2018

jklymak commented Apr 24, 2018 • edited

afvincent commented Apr 24, 2018

jklymak commented Apr 24, 2018

jklymak commented Apr 24, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jklymak Apr 24, 2018 • edited

Choose a reason for hiding this comment

afvincent Apr 24, 2018 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

afvincent commented Apr 24, 2018

afvincent commented Apr 24, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jklymak commented Apr 24, 2018

afvincent left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

afvincent commented Apr 24, 2018

jklymak commented Apr 28, 2018

afvincent left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jklymak commented May 3, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jklymak commented May 7, 2018

lumberbot-app bot commented May 7, 2018

jklymak commented May 7, 2018

afvincent commented Apr 12, 2018 •

edited

afvincent commented Apr 24, 2018 •

edited by jklymak

jklymak commented Apr 24, 2018 •

edited

jklymak Apr 24, 2018 •

edited

afvincent Apr 24, 2018 •

edited