Movatterモバイル変換


[0]ホーム

URL:


Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

Sign up
Appearance settings

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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.

Already on GitHub?Sign in to your account

Merged
QuLogic merged 1 commit intomatplotlib:mainfromStefRe:fix/autopos_title
Jan 6, 2022

Conversation

StefRe
Copy link
Contributor

@StefReStefRe commentedDec 29, 2021
edited
Loading

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 (andpytest passes).
  • IsFlake 8 compliant (installflake8-docstrings and runflake8 --docstring-convention=all).

Documentation

  • [N/A] New features are documented, with examples if plot related.
  • [N/A] New features have an entry indoc/users/next_whats_new/ (follow instructions in README.rst there).
  • API changes documented indoc/api/next_api_changes/ (follow instructions in README.rst there).
  • [N/A] Documentation is sphinx and numpydoc compliant (the docs shouldbuild 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 ofyaxis. 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
ContributorAuthor

@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 forall titles, not just for the ones that would overlap:

importmatplotlib.pyplotaspltfig,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

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
ContributorAuthor

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

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.

yuanx749 reacted with thumbs up emoji

@StefRe
Copy link
ContributorAuthor

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

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

@StefRe
Copy link
ContributorAuthor

@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)
fortitleintitles:

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]fortitleintitles)
fortitleintitles:
# 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

@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 shouldalways 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 shouldalways 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

@jklymakjklymak left a comment
edited
Loading

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:

importmatplotlib.pyplotaspltfig,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 spillingwell into the axes above.

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

@StefRe
Copy link
ContributorAuthor

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 pltfig, 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
ContributorAuthor

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

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.

@jklymakjklymak added the topic: geometry managerLayoutEngine, Constrained layout, Tight layout labelJan 5, 2022
@jklymakjklymak added this to thev3.6.0 milestoneJan 5, 2022
@jklymak
Copy link
Member

jklymak commentedJan 5, 2022
edited
Loading

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

@jklymakjklymak marked this pull request as draftJanuary 5, 2022 08:50
@StefReStefReforce-pushed thefix/autopos_title branch 2 times, most recently from7182602 tocbec785CompareJanuary 5, 2022 13:31
@StefReStefRe marked this pull request as ready for reviewJanuary 5, 2022 15:12
Move any title above the y axis offset text it would overlap with theoffset. If multiple titles are present, they are vertically aligned tothe highest one.
@QuLogicQuLogic merged commit980f313 intomatplotlib:mainJan 6, 2022
@StefReStefRe deleted the fix/autopos_title branchJanuary 6, 2022 07:08
Sign up for freeto join this conversation on GitHub. Already have an account?Sign in to comment
Reviewers

@jklymakjklymakjklymak approved these changes

@QuLogicQuLogicQuLogic approved these changes

Assignees
No one assigned
Labels
topic: geometry managerLayoutEngine, Constrained layout, Tight layout
Projects
None yet
Milestone
v3.6.0
Development

Successfully merging this pull request may close these issues.

[Bug]: Autopositioned title overlaps with offset text
4 participants
@StefRe@yuanx749@jklymak@QuLogic

[8]ページ先頭

©2009-2025 Movatter.jp