Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Already on GitHub? Sign in to your account

NF - Left and right side axes titles #1644

Merged
merged 1 commit into from Jan 26, 2013

Conversation

Projects
None yet
8 participants
Contributor

ajdawson commented Jan 8, 2013

Add left and right side titles to axes, in addition to the existing title which is in the center.

There are many instances in which one would like to display title on the left and right sides of the top of an axes rather than in the center (e.g., title on the left side and some relevant statistic on the right side). This PR adds two new titles to matplotlib axes to allow this (in addition to the existing title functionality).

A quick example:
lrtitles

@dmcdougall dmcdougall commented on an outdated diff Jan 15, 2013

lib/matplotlib/pyplot.py
@@ -1185,7 +1185,43 @@ def title(s, *args, **kwargs):
draw_if_interactive()
return l
+def lefttitle(s, *args, **kwargs):
+ """
+ Set the left-side title of the current axis.
@dmcdougall

dmcdougall Jan 15, 2013

Member

I think I would prefer current axes rather than current axis.

@dmcdougall dmcdougall commented on an outdated diff Jan 15, 2013

lib/matplotlib/pyplot.py
+def righttitle(s, *args, **kwargs):
+ """
+ Set the right-side title of the current axis.
@dmcdougall

dmcdougall Jan 15, 2013

Member

I think I would prefer current axes rather than current axis.

Member

dmcdougall commented Jan 16, 2013

This is a nice addition to matplotlib's feature set. Furthermore, it is very well documented and succinctly coded. Your effort and time taken to produce this work is greatly appreciated. I am especially impressed with the fact you have gone above and beyond by providing an example and tests, including a test with tight_layout. Excellent work.

Would you be able to address the (few) nitpicks I have with a couple of the docstrings? Also, for this to merge cleanly it needs to be rebased onto the current master branch.

Contributor

ajdawson commented Jan 16, 2013

@dmcdougall I would prefer current axes to current axis in the docstrings too, but I chose to be consistent with the matplotlib.pyplot.title docstring. I think they should all use current axes, and am happy to make this change if you agree.

Contributor

NelleV commented Jan 16, 2013

I'm actually 👎 on such a feature. There are already way too many methods for a normal human being to remember on the axes class (the class itself takes around 7kloc, and the file contains more than 400methods), and adding more doesn't seem to me like a good idea. I think it makes much more sense to add a well documented argument to set_title.

Contributor

ajdawson commented Jan 16, 2013

Sure you could move the existing title. The problem I am trying to solve is when more than one title is required. I want to make it easy to produce a plot like:

title_example

[This graphic was produced with the NCAR command language (NCL)]

In my experience this type of labelling requirement is fairly common and supported out of the box so to speak in other packages (e.g., NCAR Command Language, the Climate Data Analysis Tools). My aim here is to make life easy for those users wishing to switch to matplotlib from another plotting package.

Contributor

NelleV commented Jan 16, 2013

If I understand correctly, what you are trying to do is to position axis label differently than what is done right now in matplotlib, and not position the title differently. So in any case, the name is quite wrong IMO.
Maybe set_xlabel and set_ylabel should be able to do that.

Also, I think trying to mimic other packages is not a good way to go: to quote someone; "I realize that
trying to imitate another package squashes much innovation and does not lead to well-designed or Pythonic solutions.". I think it is a good idea to make life easier for people wanting to switch from another plotting package to matplotlib. I just don't think that adding many methods is the right way to go. Cluttering the API with many methods just makes it harder for everyone (including matplotlib developpers) to know which one to use: it is a common complaint that matplotlib has too many methods. Let's try to improve the one that exists, instead of adding others.

I here express my personal point of view. I've been contributing to matplotlib only for a few of months now, and I am not sure what I said here corresponds to matplotlib's philosophy.

Contributor

ajdawson commented Jan 16, 2013

If I understand correctly, what you are trying to do is to position axis label differently than what is done right now in matplotlib, and not position the title differently. So in any case, the name is quite wrong IMO.
Maybe set_xlabel and set_ylabel should be able to do that.

You have misunderstood. These are titles, performing the same function as a centred title, but in a different location!

As a matplotlib user I became fed-up of adding custom text/annotations to serve this purpose and felt that it is something matplotlib should be able to handle. I understand that it is sometimes confusing for new users to see all these methods, that is why I added an example.

I know there are a lot of methods in the axes API, but surely it is not sensible to reject new features that users would like to use because the class is "full"...

Member

pelson commented Jan 16, 2013

Personally it has my 👍 - my only reservation is whether to go for left_title instead of lefttitle (my preference is probably the former, but I think is also probably the least consistent).

As for @NelleV 's concerns - I fully see what your saying, but for me these functions make more sense than many of the others which exist in pyplot / on an Axes and I genuinely believe that the proposed changes are beneficial in the context provided.

@NelleV - I'm very pleased we are able to share our opinions freely, challenge the status quo and openly disagree with one another - I think it makes for a very healthy atmosphere and long may it continue!

Contributor

ajdawson commented Jan 16, 2013

@pelson I wasn't sure about lefttitle vs left_title. My opinion was that although left_title is more readable, lefttitle appears to be more consistent with the rest of the naming in matplotlib. This can of course up for debate should anyone feel differently.

Contributor

NelleV commented Jan 16, 2013

As @pelson and @dmcdougall both seem to agree on the fact that these features should go in, here are my remarks.

I agree with @pelson on the fact that the attribute lefttitle should be replaced by left_title.

Also, because there is a get_lefttitle method, maybe the attribute should be private: this would help not to clutter the API. I have mixed feeling about this, because the get methods are not pythonic, and the attribute title is public.

Last but not least, I'm currently working on MEP10, which reformats the documentation and I would greatly appreciate if these new methods' documentation could follow this convention (numpy's documentation convention). I can submit a PR on your branch to do this, if you prefer.

@NelleV NelleV and 1 other commented on an outdated diff Jan 16, 2013

examples/pylab_examples/titles_demo.py
@@ -0,0 +1,15 @@
+#!/usr/bin/env python
+"""
+matplotlib can display plot titles above axes in the center, to the left and
@NelleV

NelleV Jan 16, 2013

Contributor

I find this part of the documentation confusing. I think the term "axes" should be dropped here, because it can refer both to the matplotlib object, and the mathematical object "axis". (If I am not mistaken, in english, axes is the plural of axis)

@ajdawson

ajdawson Jan 16, 2013

Contributor

I agree axes can be confusing, as here it is in the axes object sense. Since this is in a docstring of an example which will have the example plot above it, I thought it did not matter so much...

Member

pelson commented Jan 16, 2013

Also, because there is a get_lefttitle method, maybe the attribute should be private: this would help not to clutter the API. I have mixed feeling about this, because the get methods are not pythonic, and the attribute title is public.

Agreed. The get_ set_ approach is a major bug-bear of mine too, so your not alone on making these stylistic compromises and feeling bad about it 😄 .

I think the term "axes" should be dropped here, because it can refer both to the matplotlib object, and the mathematical object "axis"

Again, agreed. Sadly we have Axes objects which represent a sub area of the figure with its own coordinate system (on which you add lines, contours, images etc.) - in many ways, this is the "plot" area where it is possible to have multiple Axes or plots per figure. Axes objects contain at least two Axis objects, which represent the numbers, tick marks and gridlines of the x and y axes. We are where we are (FWIW I don't like it either), and this terminology is engrained into mpl - I don't think we should avoid using it.

Member

dmcdougall commented Jan 16, 2013

This needs rebasing and force-pushing.

Once this is in a state where @NelleV, @ajdawson, and @pelson are mostly in agreement I think it can be merged.

Owner

mdboom commented Jan 16, 2013

I'm only skimming these PRs, but as a sanity check: does this have any relationship to #1519?

Contributor

ajdawson commented Jan 16, 2013

@mdboom The short answer is no.

Owner

efiring commented Jan 16, 2013

I'm late in commenting, but I think @NelleV was suggesting the the user interface could be handled by adding a kwarg to the Axes.set_title() and get_title() methods:

gca().set_title("Middle") # default is loc="center"
gca().set_title("Left", loc="left")
gca().set_title("Right"), loc="right")

instead of adding two new methods. This seems to me like a reasonable, and possibly superior, alternative, that would not require huge changes to the PR. Was it considered and rejected?
Or how about adding a single method like this:

gca().set_titles("Left", "Middle", "Right") # sets all three
gca().set_titles("", "Middle", "Right") # erases any left title, sets others
gca().set_titles(None, "Middle") # Sets only center, leaves sides alone

or another modification of set_title() or a variation on set_titles():

gca().set_title("Middle", left="Left", right="Right")
Member

WeatherGod commented Jan 16, 2013

I like @efiring's suggestion. I think it is much clearer (I wasn't liking
the originally proposed names), and it is really easy to discover this new
feature by checking out the call signature. The only draw-back I see is
how to handle setting the text properties independently.

Contributor

ajdawson commented Jan 16, 2013

I'm going to modify to use Axes.set_title and Axes.get_title only and use a keyword argument loc to control which title is being worked with as @efiring suggested. I can make Axes.lefttitle into Axes._left_title (and equivalent for right) so that they do not clutter the API, but I'll have to leave Axes.title as it is part of the public API already. Any comments on this? If not I will update soon and the discussion can begin again 😄

Member

dmcdougall commented Jan 16, 2013

The only draw-back I see is how to handle setting the text properties independently.

Yes. How do we do this and allow the user to set, say, the left title without having to set the main (middle) title.

Member

WeatherGod commented Jan 16, 2013

I think Andrew's suggestion would actually work quite well if we have
loc='center' as the default for set_title(). This way, the properties are
set on a per-title basis. And I am fine with Axes._lefttitle and
Axes._righttitle. We just need to make sure that the bbox='tight' code
works properly for savefig(), (it has to be told which artists to check).

Contributor

ajdawson commented Jan 16, 2013

@WeatherGod I think I have set those artists already so I think it is fine.

However, I have just thought, if we don't use separate methods for each title won't we lose the ability to use setp/getp to handle these properties?

Contributor

ajdawson commented Jan 16, 2013

Also, if mpl does move to properties (ongoing discussion on dev mailing list), wont we need a separate getter/setter for each title? I'm now a bit unsure about using only one method to control 3 separate titles...

Owner

efiring commented Jan 16, 2013

On 2013/01/16 10:58 AM, Andrew Dawson wrote:

@WeatherGod https://github.com/WeatherGod I think I have set those
artists already so I think it is fine.

However, I have just thought, if we don't use separate methods for each
title won't we lose the ability to use |setp|/|getp| to handle these
properties?

Yes, that's a disadvantage, and maybe an important one. getp(obj) is
somewhat nice for seeing an object's properties. getp and setp are both
Matlab-isms for getting and setting actual properties. We are stuck
with them, but I'm not sure whether all properties, for all time, have
to be accessible that way.


Reply to this email directly or view it on GitHub
#1644 (comment).

Member

dmcdougall commented Jan 18, 2013

How do we move forward? I think the idea of the keyword argument in lieu of [gs]etters as suggested by @NelleV and @efiring is great. However, I'd rather not lose the flexibility of setting the properties of each text instance separately.

@dmcdougall dmcdougall was assigned Jan 18, 2013

Contributor

ajdawson commented Jan 18, 2013

How do we move forward? I think the idea of the keyword argument in lieu of [gs]etters as suggested by @NelleV and @efiring is great.

From my perspective as a user I'd prefer to keep these as separate methods, it makes it very clear where to find the functionality I'm after. However, with my software engineering hat on I can see that using a keyword argument loc in set_title might be cleaner, although I am still concerned that it does depart somewhat from the established system of being able to retrieve and set properties using getp/setp...

However, I'd rather not lose the flexibility of setting the properties of each text instance separately.

If we went for keyword argument to set_title there would be no loss of generality regarding being able to set each title's properties separately, so don't worry about that. We just won't be able to do it using setp.

Contributor

NelleV commented Jan 18, 2013

I think MEP13 needs to be taken in account here: https://github.com/matplotlib/matplotlib/wiki/MEP13

I have been totally swamped with meetings and teaching, so I did not have the time to look at what the MEP implied, so maybe someone else can review.

Contributor

ajdawson commented Jan 18, 2013

Yes MEP13 is what I was referring to earlier (before it became MEP13). I think MEP13 implies that each title should be a property, that is have its own getter/setter methods. Of course MEP13 is still preliminary but it would be worth bearing in mind.

Member

dmcdougall commented Jan 18, 2013

If we went for keyword argument to set_title there would be no loss of generality regarding being able to set each title's properties separately, so don't worry about that. We just won't be able to do it using setp.

Ahhh! There's still an axes.Axes.left_title, say. I see. Personally, I have never used [gs]etp. Now, I know that's not gospel to remove or not cater for it, but it does influence my opinion nonetheless; I still prefer the kwarg approach.

Contributor

ajdawson commented Jan 19, 2013

OK I've decided to go with the kwarg approach. This means that the left and right titles will not be able to be used with getp/setp, but in turn no new methods have been added to the matplotlib API. If in the future MEP13 is applied, then the Axes.set_title method can be broken down into separate properties for each title, which won't be too much work as the underlying instance variables for each title are still present.

@NelleV: I have used the new numpy docstring format to Axes.set_title and pyplot.title, but I'm a little unsure of some things like the current state of cross referencing in the docs. Could you review please.

@dmcdougall: I'll squash these commits if this latter method is accepted, but it probably needs rebasing before merge anyway.

Member

dmcdougall commented Jan 19, 2013

I think this new implementation is cleaner.

@pelson @NelleV @efiring @WeatherGod You guys have made comments on this PR. What do you think of this new approach?

@efiring efiring commented on an outdated diff Jan 19, 2013

doc/users/whats_new.rst
@@ -49,6 +49,13 @@ using Inkscape for example, while preserving their intended position. For
`svg` please note that you'll have to disable the default text-to-path
conversion (`mpl.rc('svg', fonttype='none')`).
+Left and right side axes titles
+-------------------------------
+Andrew Dawson added the ability to add axes titles flush with the left and
+right sides of the top of the axes using :func:`~matplotlib.pyplot.lefttitle`
@efiring

efiring Jan 19, 2013

Owner

This whats_new entry will need updating for the new implementation.

Owner

efiring commented Jan 19, 2013

I think there is no perfect solution, and we could discuss it endlessly; but this one looks good enough to go with it. It represents the addition of a very useful feature in a way that is non-disruptive and, as far as I can see, that doesn't interfere with any future direction in which we might decide the API should involve. (I haven't actually tested the code myself.)

Member

pelson commented Jan 21, 2013

👍

@NelleV NelleV and 1 other commented on an outdated diff Jan 21, 2013

lib/matplotlib/tests/test_text.py
@@ -146,4 +146,14 @@ def test_contains():
vl = ax.viewLim.frozen()
ax.plot(x, y, 'o', color=color)
ax.viewLim.set(vl)
-
+
+
+@image_comparison(baseline_images=['titles'])
+def test_titles():
+ # left and right side titles
+ fig = plt.figure()
+ ax = plt.subplot( 1, 1, 1 )
@NelleV

NelleV Jan 21, 2013

Contributor

PEP8 nitpick: you've got extra spaces after and before (, )

@ajdawson

ajdawson Jan 21, 2013

Contributor

Fixed. Also fixed in the test I copy-and-pasted this line from.

Contributor

NelleV commented Jan 21, 2013

LGTM

Member

dmcdougall commented Jan 21, 2013

This needs another rebase.

@ajdawson ajdawson NF - Left and right side axes titles
Add left and right side titles to a set of axes, in addition to the
existing title which is in the center.
917f3ae
Contributor

ajdawson commented Jan 22, 2013

@dmcdougall this is rebased onto the current master and squashed into a single commit.

@dmcdougall dmcdougall added a commit that referenced this pull request Jan 26, 2013

@dmcdougall dmcdougall Merge pull request #1644 from ajdawson/left-right-titles
NF - Left and right side axes titles
2f86924

@dmcdougall dmcdougall merged commit 2f86924 into matplotlib:master Jan 26, 2013

1 check failed

default The Travis build failed
Details
Member

dmcdougall commented Jan 26, 2013

Cheers @ajdawson.

Contributor

mspacek commented Mar 17, 2013

I've discovered that this PR is responsible for the complete breakage of the qt4_editor reported in #1714. Specifically, Axes.get_title now returns a matplotlib.text.Text instance instead of a simple Python string as before. Was this intentional, to conform with MEP13 or something? Other than the call in figureoptions.py, were all calls to Axes.get_title changed accordingly? This will break a lot of user code that taps into the MPL API, won't it? Shouldn't this change have been documented in api_changes.rst?

Contributor

ajdawson commented Mar 17, 2013

This was not intentional and should be fixed.

@ajdawson ajdawson deleted the ajdawson:left-right-titles branch Jul 18, 2013

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment