FIX: move title(s) up if subscripts hang too low. by jklymak · Pull Request #11502 · matplotlib/matplotlib

jklymak · 2018-06-25T03:58:12Z

PR Summary

Partially adresses #11468. See also #11499 for multiline fixes...

EDIT:

Currently if a title has a large subscript, usually because it is math text (though large title font sizes can do the same thing), it will overhang the axes.

This PR adjusts the title position so this doesn't happen. It moves all three titles (left, right and centre) to the same level to maintain consistency of the baseline on the axes.

Note that the new version can overspill the top of the figure unless tight_layout or constrained_layout are used. But that overspill is still better than spilling down over the axes in my opinion.

Code

import matplotlib
import matplotlib.pyplot as plt

fig, axs = plt.subplots(1, 2, figsize=(9, 2.5), constrained_layout=True)
ax = axs[0]
ax.set_title('$\sum_{i} x_i$')
ax.set_title('New way', loc='left')
ax.set_xticklabels('')

ax = axs[1]
tt = ax.set_title('$\sum_{i} x_i$')
x, y = tt.get_position()
tt.set_position((x, 1.01))
ax.set_title('Old Way', loc='left')
ax.set_xticklabels('')
plt.show()

PR Checklist

Has Pytest style unit tests
Code is PEP 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

jklymak · 2018-06-25T14:03:11Z

OK, so this "only" fails 50 tests - all of which have titles (i.e. about 16 individual tests). If this is an OK approach I will regenerate the test images.

Pros: large subscript titles won't drop down below the top of the axes.

Cons: The baseline of a title like "platypus" would be displaced vertically from the top of the axes relative to a title like "aardvark"

timhoffm · 2018-06-27T07:42:33Z

I don't think the con is even true:

fd_bottom = {'verticalalignment': 'bottom'}
fd_baseline = {'verticalalignment': 'baseline'}

_, axs = plt.subplots(1, 2)
axs[0].plot([1, 3, 2])
axs[1].plot([1, 3, 2])

axs[0].set_title('baseline', loc='left', fontdict=fd_baseline)
axs[0].set_title('ggggggg', loc='center', fontdict=fd_baseline)
axs[1].set_title('bottom', loc='left', fontdict=fd_bottom)
axs[1].set_title('ggggggggg', loc='center', fontdict=fd_bottom)

So, 👍

tacaswell · 2018-06-29T00:15:47Z

👍

Can you check if those tests are actually testing titles? Any which are not should probably have the title removed (to reduce our exposure to font-related changes in general).

Is this just a matter of tuning the padding just right to make it match?

jklymak · 2018-07-02T16:12:39Z

OK: I decided I didn't like this approach because it allowed no way to align the titles (at their baselines) without manually doing verticalalign='baseline'. This algorithm is a bit more complicated, but does the right thing (I'd argue). Note that if any of the three titles (left, centre, and right) are manually moved, this all turns off, so the user still has manual placement ability.

This is also going to fail far fewer tests....

After:

jklymak · 2018-07-02T20:51:46Z

This only fails two tests in tight_layout where for some reason the x/y line gets shifted by a point for some funky reason. It happens locally as well, so I can re-produce. Its a little funky and likely to do with pixel snapping, but is harmless to the test...

efiring

Apart from the two minor specific comments, this looks good to me.
But doesn't it need one added test like the illustrations that are given above to motivate the PR?

efiring · 2018-08-18T23:45:26Z

            x, y0 = title.get_position()
-            y = 1.0
+            y = 1
+            # need to start agin in case of window resizing


Typo in comment.

efiring · 2018-08-18T23:48:53Z

+                    y = 1
+            bt = title.get_window_extent(renderer)
+            # need to do twice to get right for manual resizing
+            for nn in range(1):


This is doing it only once. Does it really need to be done twice?

Once works!

jklymak · 2018-08-19T04:15:29Z

Thanks @efiring test added...

timhoffm · 2018-08-19T21:12:48Z

-                        y = max(y, yn)
+                    else:
+                        bb = ax.get_window_extent(renderer)
+                    top = np.max((top, bb.ymax))


Not that it's particular important, but I would just use the builtin max(). It's easier to read and also faster for just two numbers.

timhoffm · 2018-08-19T21:13:25Z

+                    y = self.transAxes.inverted().transform(
+                        (0., top - dy))[1]
+                title.set_position((x, y))
+            ymax = np.max((y, ymax))


Builtin max()?

timhoffm · 2018-08-19T21:25:04Z

+                    y = 1
+            bt = title.get_window_extent(renderer)
+            dy = bt.ymin - top
+            if dy < 0:


AFAICS this dy value is not used again. In that case, it would be more clear to:

if title.get_window_extent(renderer).ymin < top: # title overlaps the Axes ...

IMO the temporary variables do not add to the clarity of the code.

Sorry, leftovers from debugging where the temporary variables were used a few more times ;-)

timhoffm · 2018-08-19T21:27:07Z

+                y = self.transAxes.inverted().transform(
+                        (0., top))[1]
+                title.set_position((x, y))
+                bt = title.get_window_extent(renderer)


Can you please add a short comment, why you do the setting of the position in two steps?

Hmmm, yes, it should be exactly the same w/ or w/o the extra block, but emperically its not with the state that get_extents is in now... 😢 Comment added...

timhoffm

I'll leave it up to you, if you want to update the max() usages.

timhoffm · 2018-08-21T21:05:50Z

-                        y = max(y, yn)
+                    else:
+                        bb = ax.get_window_extent(renderer)
+                    top = max((top, bb.ymax))


max(top, bb.ymax) would work as well, i.e. multiple arguments instead of a tuple. It's even slightly more clean.

TST: Add test for large subscript

jklymak · 2018-08-21T22:03:03Z

Thanks @timhoffm , fixed!

tacaswell added this to the v3.1 milestone Jun 29, 2018

jklymak added the status: work in progress label Jul 2, 2018

jklymak force-pushed the fix-title-bottom-vs-baseline branch from 2c7594e to 7108a60 Compare July 2, 2018 16:09

jklymak changed the title ~~FIX: move title verticalalignment to bottom from baseline~~ FIX: move title(s) up if subscripts hang too low. Jul 3, 2018

jklymak removed the status: work in progress label Jul 3, 2018

jklymak force-pushed the fix-title-bottom-vs-baseline branch from d44c84d to 9e0a8b3 Compare July 3, 2018 20:25

jklymak mentioned this pull request Jul 8, 2018

Layout managers have problems with titles containing MathText #11468

Closed

efiring requested changes Aug 18, 2018

View reviewed changes

jklymak force-pushed the fix-title-bottom-vs-baseline branch 2 times, most recently from ac877a4 to cb636e6 Compare August 19, 2018 04:18

efiring approved these changes Aug 19, 2018

View reviewed changes

jklymak added the status: needs review label Aug 19, 2018

timhoffm reviewed Aug 19, 2018

View reviewed changes

jklymak force-pushed the fix-title-bottom-vs-baseline branch from cb636e6 to a04c64c Compare August 20, 2018 16:27

timhoffm approved these changes Aug 21, 2018

View reviewed changes

FIX: move titles up if overlap axes

e38c544

TST: Add test for large subscript

jklymak force-pushed the fix-title-bottom-vs-baseline branch from a04c64c to e38c544 Compare August 21, 2018 22:02

timhoffm merged commit ebb6aa7 into matplotlib:master Aug 22, 2018

timhoffm removed the status: needs review label Aug 22, 2018

ImportanceOfBeingErnest mentioned this pull request Apr 2, 2019

constrained_layout (sometimes) clips multiline title/xlabel/ylabel #13843

Closed

jklymak mentioned this pull request Mar 22, 2020

FIX: turn off title autopos if pad is set #16862

Closed

6 tasks

Conversation

jklymak commented Jun 25, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

PR Summary

Code

PR Checklist

Uh oh!

jklymak commented Jun 25, 2018

Uh oh!

timhoffm commented Jun 27, 2018

Uh oh!

tacaswell commented Jun 29, 2018

Uh oh!

jklymak commented Jul 2, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

After:

Uh oh!

jklymak commented Jul 2, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

efiring left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jklymak commented Aug 19, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

timhoffm Aug 19, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jklymak Aug 20, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

timhoffm left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jklymak commented Aug 21, 2018

Uh oh!

Reviewers

Uh oh!

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

jklymak commented Jun 25, 2018 •

edited

Loading

jklymak commented Jul 2, 2018 •

edited

Loading

jklymak commented Jul 2, 2018 •

edited

Loading

timhoffm Aug 19, 2018 •

edited

Loading

jklymak Aug 20, 2018 •

edited

Loading