Skip to content

Move title up if x-axis is on the top of the figure #9498

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 2 commits into from
Apr 3, 2018

Conversation

jklymak
Copy link
Member

@jklymak jklymak commented Oct 21, 2017

PR Summary

It the x-axis is at the top of the axes, then the title is actually rendered in the middle of the axis labels.

This version works no matter the size of the figure (unlike the version that @tacaswell was 50/50 on below), though tight_layout (or #9082) are needed to get the title to show with the default axes positions...

import matplotlib.pyplot as plt

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

ax.xaxis.tick_top()
ax.xaxis.set_label_position('top')

ax.set_xlabel('Xlabel')
ax.set_title('Title')

Before:

withfix

After:

fixtitle

The title position is changed at draw time based on the top of the x-axis or the xlabel. If the user has manually placed the title (ax.title.get_position((x,y))) then the title won't be moved unless they happened to place it at y=1.0. This degeneracy can be fixed by setting y=1.001. (This very slight annoyance avoids having to make a new method to set the title that would not be backward compatible).

This is a fix to #1415

Note: based on axis._update_label_position...

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)

@tacaswell tacaswell added this to the v2.2 milestone Oct 21, 2017
@tacaswell
Copy link
Member

I am 50/50 on this. This is a simple fix to a long standing bug, but it is only partial (as you explain) and hysteresis in APIs in one of the things that frustrates users the most.

This seems like something that #9082 should take care of? If the goal is to get that in for 2.2, I don't think we should merge this.

@jklymak
Copy link
Member Author

jklymak commented Oct 21, 2017

@tacaswell #9082 is limited to the same functionality as tight_layout right now in that it only operates at the axes level and above, and not inside axes. So it can’t handle this case

Not to say it couldn’t handle this case with more work. But there is a bit of a conceptual leap from figure space to axes space. Axes layout can almost be done as a separate pass from figure layout.

@jklymak
Copy link
Member Author

jklymak commented Oct 21, 2017

Btw totally fine if this isn’t accepted. If it often requires manual intervention anyways perhaps folks should just set manually. I’ll poke around for a good part of the tutorials to indicate how that can be done.

@jklymak jklymak closed this Oct 21, 2017
@jklymak
Copy link
Member Author

jklymak commented Oct 21, 2017

OK, closing. I'm going to copy what xlabel and ylabel do and move the title just above whatever is at the top of the upper spine. It has some confusing transforms, but I think its totally doable.

@jklymak jklymak reopened this Oct 22, 2017
@jklymak
Copy link
Member Author

jklymak commented Oct 22, 2017

Re-opening! Please see new description above.

@jklymak
Copy link
Member Author

jklymak commented Oct 22, 2017

@tacaswell Just to carry on the discussion above: the new solution is basically what a geometry manager would do anyway: put the title above the xlabel. I'd argue that a constraint manager isn't particularly needed for such a simple task. Now, if you wanted all the titles for different subplots to line up that is when a more expansive geometry manager would be helpful.

@jklymak jklymak force-pushed the fixtitle branch 3 times, most recently from c28f1f4 to 7c9ab29 Compare October 22, 2017 21:16
@jklymak jklymak changed the title Move title up if x-axis is on the top of the figure MRG: Move title up if x-axis is on the top of the figure Nov 1, 2017
@jklymak jklymak mentioned this pull request Nov 1, 2017
9 tasks
@jklymak jklymak requested review from dstansby and efiring November 2, 2017 21:55
@jklymak
Copy link
Member Author

jklymak commented Jan 29, 2018

rebase

@jklymak
Copy link
Member Author

jklymak commented Feb 2, 2018

@efiring changes broke this, but I'm sure they are fixable, just will take a bit of work...

@afvincent
Copy link
Contributor

Well, I may be naive, but is it a lot more complicated than replacing

axs = [self] + self._twinx_axes + self._twiny_axes

by

axs = self._twinned_axes.get_siblings(self)
# NB:
#
# In [2]: ax._twinned_axes.get_siblings?
# Signature: ax._twinned_axes.get_siblings(a)
# Docstring: Returns all of the items joined with *a*, including itself.

as well as in the tests

self._twinx_axes.append(ax2)
# and
self._twiny_axes.append(ax2)

by

self._twinned_axes.join(self, ax2)

?

@afvincent
Copy link
Contributor

With the following patch, the new tests are passing locally. I have one remaining failure in all the tests related to axes, about one of the boxplot pictures, but it looks like it is failure for a few pixel that are a bit off.

From add5ce3c1a078164657e4d8b9dd58eda68f7275b Mon Sep 17 00:00:00 2001
From: "Adrien F. Vincent" <vincent.adrien@gmail.com>
Date: Fri, 2 Feb 2018 13:56:14 -0800
Subject: [PATCH 2/2] use new privates attributes about twins

---
 lib/matplotlib/axes/_base.py | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/lib/matplotlib/axes/_base.py b/lib/matplotlib/axes/_base.py
index da85db0d5..15cc47c96 100644
--- a/lib/matplotlib/axes/_base.py
+++ b/lib/matplotlib/axes/_base.py
@@ -2507,7 +2507,7 @@ class _AxesBase(martist.Artist):
             x, y0 = title.get_position()
             y = 1.0
             # need to check all our twins too...
-            axs = [self] + self._twinx_axes + self._twiny_axes
+            axs = self._twinned_axes.get_siblings(self)
 
             for ax in axs:
                 try:
@@ -4243,7 +4243,7 @@ class _AxesBase(martist.Artist):
         self.yaxis.tick_left()
         ax2.xaxis.set_visible(False)
         ax2.patch.set_visible(False)
-        self._twinx_axes.append(ax2)
+        self._twinned_axes.join(self, ax2)
         return ax2
 
     def twiny(self):
@@ -4273,7 +4273,7 @@ class _AxesBase(martist.Artist):
         self.xaxis.tick_bottom()
         ax2.yaxis.set_visible(False)
         ax2.patch.set_visible(False)
-        self._twiny_axes.append(ax2)
+        self._twinned_axes.join(self, ax2)
         return ax2
 
     def get_shared_x_axes(self):
-- 
2.14.3

I am not totally confident on how to properly push that fix against the current PR branch and avoid breaking @jklymak stuff :/.

@jklymak
Copy link
Member Author

jklymak commented Feb 2, 2018

Yeah, I don't know how to force-push either ;-) Thanks for the patch, I'll check it in by tomorrow....

@jklymak jklymak force-pushed the fixtitle branch 2 times, most recently from de80a64 to ea54c71 Compare March 23, 2018 18:04
@jklymak
Copy link
Member Author

jklymak commented Mar 23, 2018

recycling. SOmething funny w/ CI

@jklymak
Copy link
Member Author

jklymak commented Mar 23, 2018

Added test for no-xlabel (thanks @afvincent, per gitter). Fixed bug for that case as well (typo)

@jklymak
Copy link
Member Author

jklymak commented Mar 31, 2018

ping @afvincent - this seem OK to you now? Thanks!

Copy link
Contributor

@afvincent afvincent left a comment

Choose a reason for hiding this comment

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

Disclaimer: I am way too out of my depth to judge the logging part...

Otherwise, apart from a few typos (I think), a comment about the semantic of y1 vs ymax in boundary boxes, and a few other remarks mainly made by curiosity, this starts to look good to me :).


Previously the axes title had to be moved manually if an xaxis overlapped
(usually when the xaxis was put on the top of the axes). The title can
still be placed manaully.
Copy link
Contributor

Choose a reason for hiding this comment

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

manually?

if (ax.xaxis.get_label_position() == 'top'
or ax.xaxis.get_ticks_position() == 'top'):
bb = ax.xaxis.get_tightbbox(renderer)
top = bb.y1
Copy link
Contributor

Choose a reason for hiding this comment

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

Should it not be top = bb.ymax rather than top = bb.y1? That is a very genuine question, because I am not that used to play with boundary boxes and the docstring of y1 reads

:attr:`y1` is the second of the pair of *y* coordinates that
define the bounding box. :attr:`y1` is not guaranteed to be greater
than :attr:`y0`.  If you require that, use :attr:`ymax`.

Copy link
Member Author

Choose a reason for hiding this comment

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

Oh, cool, I didn't know that.

Copy link
Contributor

Choose a reason for hiding this comment

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

Do we prefer to stay with top = bb.y1, while it actually means top = bb.ymax (if I correctly understood the process)? I guess that in 99.99% of the cases here, it will be equivalent, but the semantic of the latter might be clearer, right? (I have no strong feelings about any of the two options)

# in __init__: titleOffsetTrans
yn = self.transAxes.inverted().transform((0., top))[1]
y = max(y, yn)
except AttributeError:
Copy link
Contributor

Choose a reason for hiding this comment

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

By curiosity, when is an AttributeError likely to occur? In a case where an empty boundary box would be returned (I mean a case where bb would be None)?

Copy link
Member Author

Choose a reason for hiding this comment

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

Ummm, I forget why exactly, but I think it was possible for ax to not have an xaxis property. I can check what test fails if important.

Copy link
Contributor

Choose a reason for hiding this comment

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

Fair enough for me, no worry ;).

if self._autotitlepos is None:
for title in titles:
x, y = title.get_position()
if not np.isclose(y, 1.0):
Copy link
Contributor

Choose a reason for hiding this comment

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

Very dumb question incoming 🐑... With this implementation, there is no way to pin the title y-position at 1.0 and still avoid the auto title positioning, is there not? But in case anybody would like to do so (for whichever good or bad reason ^^), I guess that a reasonable workaround would be to pin the title y-position at something like 1.001, right?

Copy link
Member Author

Choose a reason for hiding this comment

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

Thats correct. I could do something like find the first call that sets the title at 1.0 and then flag future calls to set_position as being "pinned", even if it is y=1.0. But setting to 1.01 will give the same result.

Copy link
Contributor

Choose a reason for hiding this comment

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

IMHO, setting y=1.001 is a good enough solution. Maybe it should be put in the “what's new” entry (just for the workaround to be documented somewhere)?

Copy link
Member Author

Choose a reason for hiding this comment

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

Done!

@@ -2594,6 +2647,7 @@ def draw(self, renderer=None, inframe=False):
renderer.stop_rasterizing()

mimage._draw_list_compositing_images(renderer, self, artists)
self._update_title_position(renderer)
Copy link
Contributor

Choose a reason for hiding this comment

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

Why exactly is it called a second time in draw (first call at l. 2598)?

Copy link
Member Author

Choose a reason for hiding this comment

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

Seems to work w/o the second call. Good catch!



def test_title_xticks_top():
# Test that title stays put if we set it manually
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this comment/docstring correct?

ax.xaxis.set_ticks_position('top')
ax.set_title('xlabel top')
fig.canvas.draw()
assert ax.title.get_position()[1] > 1.04
Copy link
Contributor

Choose a reason for hiding this comment

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

Just by curiosity, is 1.04 an arbitrary threshold value that was empirically found? (I would have genuinely expected something like > 1.0, or maybe > 1.01 to avoid numerical precision issues 🐑 )

Copy link
Member Author

Choose a reason for hiding this comment

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

1.04 was just large enough to definitely not be at 1.0. No good reason for it.

Copy link
Contributor

Choose a reason for hiding this comment

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

Fair enough, I just wanted to be sure that I was not missing something.

@jklymak jklymak force-pushed the fixtitle branch 4 times, most recently from 456ad43 to 1675d25 Compare April 3, 2018 02:55
Copy link
Contributor

@afvincent afvincent left a comment

Choose a reason for hiding this comment

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

A couple remaining/new remarks, but otherwise this looks good (AFAICT) :).

Previously the axes title had to be moved manually if an xaxis overlapped
(usually when the xaxis was put on the top of the axes). The title can
still be placed manually. However, titles that need to be moved are
at ``y=1.0``, so manualy placing at 1.0 will be moved, so if the title
Copy link
Contributor

Choose a reason for hiding this comment

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

I guess that 'manualy' is a typo. Besides, the whole sentence seems quite long to me with many complements: I wonder if this makes it hard to read or not (as a French native speaker, I am a bit too biased to judge long sentences with complements of complements...). Would something like

However, titles that need to be moved are
at ``y=1.0``, so manual placing at 1.0 will be moved.  If the title is to
be placed at 1.0, it should be set to something near 1.0, like 1.001.

be OK too?

Copy link
Member Author

Choose a reason for hiding this comment

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

Agreed. Changed to be more verbose, and hopefully more clear.

Previously an axes title had to be moved manually if an xaxis overlapped
(usually when the xaxis was put on the top of the axes).  Now, the title
will be automatically moved above the xaxis and its decorators (including
the xlabel) if they are at the top.

If desired, the title can still be placed manually.  There is a slight kludge;
the algorithm checks if the y-position of the title is 1.0 (the default),
and moves if it is.  If the user places the title in the default location
(i.e. ``ax.title.set_position(0.5, 1.0)``), the title will still be moved
above the x-axis.  If the user wants to avoid this, they can
specify a number that is close (i.e. ``ax.title.set_position(0.5, 1.01)``) 
and the title will not be moved via this algorithm.

if (ax.xaxis.get_label_position() == 'top'
or ax.xaxis.get_ticks_position() == 'top'):
bb = ax.xaxis.get_tightbbox(renderer)
top = bb.y1
Copy link
Contributor

Choose a reason for hiding this comment

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

Do we prefer to stay with top = bb.y1, while it actually means top = bb.ymax (if I correctly understood the process)? I guess that in 99.99% of the cases here, it will be equivalent, but the semantic of the latter might be clearer, right? (I have no strong feelings about any of the two options)

Copy link
Contributor

@afvincent afvincent left a comment

Choose a reason for hiding this comment

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

LGTM :)! I'll let the joyce of hitting the green button to somebody more comfortable than I am with the merging/backporting process ^^.

A slight comment on the “what's new” (which is BTW definitively clearer than before), but I guess it is more an English issue than anything else: is there a slight difference in meaning between “xaxis” and “x-axis”? If there is not, then there are other uses that might not be totally consistent (e.g. “xlabel”). But I would not block this PR any longer just for this.

@jklymak
Copy link
Member Author

jklymak commented Apr 3, 2018

"xaxis" was made consistent in the whats-new. Yes "xaxis" is a bit illiterate, but it'll make it easier to find in the rest of the docs.

This won't be backported, so all you do is hit the little green button if you are happy 😉 It only gets complicated if you forget to milestone the PR and it should be backported, or if you want to squash a pile of commits. But neither is the case here.

@afvincent
Copy link
Contributor

Thanks for the explanations. Then just waiting for the CI to finish running (everything was green before but this should be over in ~ 5 minutes anyway...).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants