Skip to content

FIX: Autoposition title when yaxis has offset #22063

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 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
Jan 6, 2022

Conversation

StefRe
Copy link
Contributor

@StefRe StefRe commented Dec 29, 2021

PR Summary

Move the title above the offset text if present.

OLD:
old
NEW:
new

Closes #22062.

PR Checklist

Tests and Styling

  • Has pytest style unit tests (and pytest passes).
  • Is Flake 8 compliant (install flake8-docstrings and run flake8 --docstring-convention=all).

Documentation

  • [N/A] New features are documented, with examples if plot related.
  • [N/A] New features have an entry in doc/users/next_whats_new/ (follow instructions in README.rst there).
  • API changes documented in doc/api/next_api_changes/ (follow instructions in README.rst there).
  • [N/A] Documentation is sphinx and numpydoc compliant (the docs should build without error).

@yuanx749
Copy link
Contributor

Hi @StefRe , I came across this PR, thank you. Previously I just manually adjusted the position of the title.

I think it will be great to also consider the x position of yaxis. With your current fix, the title located centered is also moved, as shown below. While in matlab, it seems that the title only get moved upwards if there will be overlap.
title

fig, ax = plt.subplots()
ax.plot([1e8, 2e8])
ax.set_title('Left', loc='left')
ax.set_title('Center')
ax.set_title('Right', loc='right')
plt.show()

@StefRe
Copy link
Contributor Author

StefRe commented Dec 30, 2021

@yuanx749 Thanks for your feedback. I don't think, however, we should strive to cover all possible cases: moving the title up to not overlap with top ticks (which is current behavior) also sets the title position for all titles, not just for the ones that would overlap:

import matplotlib.pyplot as plt
fig, ax = plt.subplots(layout='constrained')
ax.plot([1e8, 2e8], [1, 2])
ax.set_title('Left', loc='left')
ax.set_title('Center')
ax.set_title('Right', loc='right')
ax.xaxis.tick_top()
plt.show()

Figure_3

@jklymak
Copy link
Member

jklymak commented Dec 30, 2021

I'm 50/50 on this. I'm not sure we want titles in the centre to be so high so maybe only do this if the left title is not empty?

@StefRe
Copy link
Contributor Author

StefRe commented Dec 30, 2021

If we have multiple titles as in the example (which is, admittedly, seldom in practice), the different hights of the different titles would look awkward.

@jklymak
Copy link
Member

jklymak commented Dec 30, 2021

I agree. I just meant that we only move all three of the left title is occupied.

I guess a counter argument is that some centred titles can be very long, but I think most of the time folks are going to complain about all the wasted vertical space.

@StefRe
Copy link
Contributor Author

StefRe commented Dec 30, 2021

Maybe we can consider this as a separate issue and only change one thing at a time, just to not overcomplicate things:

  • right now the title gets moved above the highest top x axis tick label and x offset text
  • this PR proposes the same for the case that there is an y axis offset text
  • neither the current code not this PR check if there's indeed an overlap between the title at its given position and any axis decorations
  • if this is desired (to save space), it could be discussed as a separate issue / PR (I'm not overly enthusiastic about it); right now there's just one y position for all three locations ('center', 'left', 'right'), so I guess we'd need three y positions then?

@jklymak
Copy link
Member

jklymak commented Dec 30, 2021

I guess I'm not being clear. My proposal is you only change the y position to account for the y offset if there is a string in the left title. I'm not proposing 2 or more separate positions.

@StefRe
Copy link
Contributor Author

StefRe commented Dec 30, 2021

@jklymak I think I understood you correctly - the only thing I wanted to say is that if we move the left title only (to not overlap with an y axis offset text), then we should also change the existing code so that center and left titles are moved up just above the top x tick labels, not above the x offset text (in case there is an x offset).

Right now all titles are treated the same way:

titles = (self.title, self._left_title, self._right_title)
for title in titles:

If we want to treat the left title separately this logic must be changed a bit.

Further, all titles are currently lined up at one level:

ymax = max(title.get_position()[1] for title in titles)
for title in titles:
# now line up all the titles at the highest baseline.
x, _ = title.get_position()
title.set_position((x, ymax))

So this will also be needed to addressed (in case there are multiple titles).

This is why I suggested to deal with these things in a separate issue "Update title positions per title to move them just the necessary amount", if this is really needed.

@jklymak
Copy link
Member

jklymak commented Dec 30, 2021

@StefRe, I am not sure you did understand correctly, or I'm not understanding you. I am agreeing with you that the vertical level of all three titles should always be the same. And in general I am agreeing with fixing the overlap between a left-side title and the y-axis offset text.

However, I disagree that we should always displace all the titles if there is y-axis offset text. That is a breaking change that will affect many people, adding undesirable new vertical space to 99.9% of the users who just have a centered title.

You mention the precedent of the x-axis being taken into account, in particular when it is at the top of the axes. That is, in fact, the whole point of the automatic repositioning feature in the first place. Having the extra space was deemed useful because the top of the axes was already full of x-tick labels, and usually the xlabel itself. I don't think that applies to an offset text on the left side and most centred titles.

Just to re-iterate, I think your PR should go in as-is, except the logic should only be called if the left title string is non-empty. (Or the right title string, if the y axis is on the right, I suppose). If that is satisfied, I agree all three titles should move up together. But I don't think we should do this if the left title string is empty.

Copy link
Member

@jklymak jklymak left a comment

Choose a reason for hiding this comment

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

My suggestion is above. On the code:

import matplotlib.pyplot as plt
fig, axs = plt.subplots(2, 1, constrained_layout=True)
ax = axs[0]
ax.plot([1e8, 2e8])
ax.set_title('Left', loc='left')
ax.set_title('Center')
ax.set_title('Right', loc='right')

ax = axs[1]
ax.plot([1e8, 2e8])
ax.set_title('Center')
ax.set_title('Right', loc='right')
plt.show()

currently yields:
CurrentPR

With the suggested change it yields:
SuggestedPR

The suggested change is also not a breaking change, except if the left title is populated, in which case it is a desired change.

If you don't use constrained_layout it is even worse with the new title position spilling well into the axes above.

In addition, this saves an extra get_tightbbox if its not needed, which can be expensive.

@StefRe StefRe force-pushed the fix/autopos_title branch from 862b4a1 to da7a16a Compare January 1, 2022 17:37
@StefRe
Copy link
Contributor Author

StefRe commented Jan 1, 2022

Updated version which only moves the title if it indeed would overlap and aligns them if there are multiple titles:

  • a) move left title above offset
  • b) don't move title if it doesn't overlap
  • c) + d) same for right offset
  • e) align multiple titles at the highest one
  • f) works for any title

Figure_1


Just for the records: this PR doesn't change title movement in case of top x axis (as discussed above):

  • g) right title is above (right) top x axis offset
  • h) center title is moved too, although it wouldn't overlap

Figure_2

Demo code:

import matplotlib.pyplot as plt

fig, axs = plt.subplots(3, 2, layout='constrained', figsize=(6,8))
for ax, letter in zip(axs.flat, 'abcdef'):
    ax.text(.05, .9, letter + ')', transform=ax.transAxes)
    ax.set_ylim(1e8)

axs[0,0].set_title('Left', loc='left')

axs[0,1].set_title('Center')

axs[1,0].yaxis.tick_right()
axs[1,0].set_title('Right', loc='right')

axs[1,1].yaxis.tick_right()
axs[1,1].set_title('Center')

axs[2,0].set_title('Left', loc='left')
axs[2,0].set_title('Center')

axs[2,1].set_title('A very long center title')

fig, axs = plt.subplots(1, 2, layout='constrained', figsize=(6,3))
for ax, letter in zip(axs, 'gh'):
    ax.text(.05, .9, letter + ')', transform=ax.transAxes)
    ax.set_xlim(1e8)
    ax.xaxis.tick_top()
    
axs[0].set_title('Right', loc='right')
axs[1].set_title('Center')

@StefRe
Copy link
Contributor Author

StefRe commented Jan 1, 2022

Hm, I'm a bit at a loss: there are 7 failing tests

test_axes.py::test_errorbar[png], [svg], [pdf]
test_streamplot.py::test_direction[png]
test_axisartist_grid_helper_curvelinear.py::test_axis_direction[png]
test_mplot3d.py::test_trisurf3d[png]
test_mplot3d.py::test_stem3d[png]

for Python 3.8 + 3.9 on Ubuntu (they pass on 3.7 and 3.10 as well as for all versions on Windows and macOS) and I don't see how the failures are related to this PR:

I can't debug as I only have a Python 3.10.1 Arch and a Python 3.8 Windows computer and all tests pass here. Do you have any idea or hint how to proceed from here?

@jklymak
Copy link
Member

jklymak commented Jan 2, 2022

We are not sure what is causing the tests to fail, but they are failing on doc PRs so something has gone wrong in our library chain.

@jklymak jklymak added the topic: geometry manager LayoutEngine, Constrained layout, Tight layout label Jan 5, 2022
@jklymak jklymak added this to the v3.6.0 milestone Jan 5, 2022
@jklymak
Copy link
Member

jklymak commented Jan 5, 2022

I guess this needs an API change note; put in draft until its added, definitely feel free to pop back....

@jklymak jklymak marked this pull request as draft January 5, 2022 08:50
@StefRe StefRe force-pushed the fix/autopos_title branch 2 times, most recently from 7182602 to cbec785 Compare January 5, 2022 13:31
@StefRe StefRe marked this pull request as ready for review January 5, 2022 15:12
Move any title above the y axis offset text it would overlap with the
offset. If multiple titles are present, they are vertically aligned to
the highest one.
@StefRe StefRe force-pushed the fix/autopos_title branch from cbec785 to cfabe79 Compare January 5, 2022 16:21
@QuLogic QuLogic merged commit 980f313 into matplotlib:main Jan 6, 2022
@StefRe StefRe deleted the fix/autopos_title branch January 6, 2022 07:08
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
topic: geometry manager LayoutEngine, Constrained layout, Tight layout
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Bug]: Autopositioned title overlaps with offset text
4 participants