Skip to content

FIX: constrained_layout and repeated calls to suptitle #11035

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.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 1 commit into from
May 7, 2018
Merged

FIX: constrained_layout and repeated calls to suptitle #11035

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion lib/matplotlib/_constrained_layout.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -235,7 +235,7 @@ def do_constrained_layout(fig, renderer, h_pad, w_pad,
ax._poslayoutbox.constrain_left_margin(0, strength='weak')

# do layout for suptitle.
if fig._suptitle is not None:
if fig._suptitle is not None and fig._suptitle._layoutbox is not None:
sup = fig._suptitle
bbox = invTransFig(sup.get_window_extent(renderer=renderer))
height = bbox.y1 - bbox.y0
Expand Down
36 changes: 22 additions & 14 deletions lib/matplotlib/figure.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -678,7 +678,7 @@ def get_window_extent(self, *args, **kwargs):
"""
return self.bbox

def suptitle(self, t, *, x=.5, y=.98, **kwargs):
def suptitle(self, t, **kwargs):
"""
Add a centered title to the figure.

Expand Down Expand Up @@ -732,6 +732,11 @@ def suptitle(self, t, *, x=.5, y=.98, **kwargs):

>>> fig.suptitle('This is the figure title', fontsize=12)
"""
manual_position = ('x' in kwargs or 'y' in kwargs)

x = kwargs.pop('x', 0.5)
y = kwargs.pop('y', 0.98)
Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Member Author

Choose a reason for hiding this comment

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

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...

Copy link
Member Author

@jklymak jklymak Apr 24, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

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...

Copy link
Contributor

@afvincent afvincent Apr 24, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

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

Copy link
Member Author

Choose a reason for hiding this comment

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

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

Copy link
Contributor

Choose a reason for hiding this comment

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

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...

Copy link
Member Author

Choose a reason for hiding this comment

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

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


if ('horizontalalignment' not in kwargs) and ('ha' not in kwargs):
kwargs['horizontalalignment'] = 'center'
if ('verticalalignment' not in kwargs) and ('va' not in kwargs):
Expand All @@ -751,19 +756,22 @@ def suptitle(self, t, *, x=.5, y=.98, **kwargs):
sup.remove()
else:
self._suptitle = sup
if self._layoutbox is not None:
# assign a layout box to the suptitle...
figlb = self._layoutbox
self._suptitle._layoutbox = layoutbox.LayoutBox(
parent=figlb,
name=figlb.name+'.suptitle')
for child in figlb.children:
if not (child == self._suptitle._layoutbox):
w_pad, h_pad, wspace, hspace = \
self.get_constrained_layout_pads(
relative=True)
layoutbox.vstack([self._suptitle._layoutbox, child],
padding=h_pad*2., strength='required')
self._suptitle._layoutbox = None
if self._layoutbox is not None and not manual_position:
w_pad, h_pad, wspace, hspace = \
self.get_constrained_layout_pads(relative=True)
figlb = self._layoutbox
self._suptitle._layoutbox = layoutbox.LayoutBox(
parent=figlb, artist=self._suptitle,
name=figlb.name+'.suptitle')
# stack the suptitle on top of all the children.
# Some day this should be on top of all the children in the
# gridspec only.
for child in figlb.children:
if child is not self._suptitle._layoutbox:
layoutbox.vstack([self._suptitle._layoutbox,
child],
padding=h_pad*2., strength='required')
self.stale = True
return self._suptitle

Expand Down
40 changes: 40 additions & 0 deletions lib/matplotlib/tests/test_constrainedlayout.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -344,3 +344,43 @@ def test_constrained_layout20():
ax = fig.add_axes([0, 0, 1, 1])
mesh = ax.pcolormesh(gx, gx, img)
fig.colorbar(mesh)


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

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

fig.suptitle("Suptitle1")
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'
Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Member Author

Choose a reason for hiding this comment

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

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

Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Member Author

Choose a reason for hiding this comment

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

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.

fig, ax = plt.subplots(constrained_layout=True)

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

fig.suptitle("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``.
Copy link
Contributor

Choose a reason for hiding this comment

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

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

'''

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