Skip to content
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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

Rewrite of the entire legend documentation, including tidy ups of code and style to all things "legend". #2442

Merged
merged 2 commits into from Jan 28, 2014

Conversation

pelson
Copy link
Member

@pelson pelson commented Sep 20, 2013

First off - apologies for such a big PR.

This work represents several days effort in rationalising and extending the legend documentation throughout matplotlib. I've focused almost entirely on improving the documentation, but in the process I've also cleaned up the code and simplified some interfaces (in the interest of clearer documentation).

There are changes to some example files which may not seem relevant, but I've gone through the entire gallery to identify the necessary legend related plots, and as a result have removed some of the duplicates which shouldn't have been there in the first place.

Important:
I'd like to put an alternative perspective on the way we review this PR given how much effort this review will require. Instead of waiting for 馃憤s - I'd like to put a deadline of 14 days (4th October 2013) to have any 馃憥s, and provided any review actions are complete, either I, or some other kind soul, will merge this. I think this approach might work well for primarily documentation type changes and this is a good place to trial it out.

Thanks!

@@ -75,15 +80,23 @@ def teardown_class(cls):
def test(self):
self._func()


def cleanup(func):
Copy link
Member Author

Choose a reason for hiding this comment

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

@mdboom - pinging you here. I did this so I could have methods which get cleaned up. I've got some ideas on how to tidy up the whole suite of testing decorators, but I was doing my best to restrain from doing them in this PR. 馃槈

Copy link
Member

Choose a reason for hiding this comment

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

I think this code predated functools (Python 2.5), so I'm glad to modernize it.


To automatically generate the legend from labels::
line, = ax.plot([1, 2, 3], label='Inline label')
Copy link
Member

Choose a reason for hiding this comment

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

There's a coma after line. Is that wanted (is that valid python syntax?)?

Copy link
Member

Choose a reason for hiding this comment

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

That is to un-pack the length one list that plot returns so line is a Line2D object, not a list.

Copy link
Member

Choose a reason for hiding this comment

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

Ah, nice. Thanks

Copy link
Member

Choose a reason for hiding this comment

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

On 2013/09/20 3:44 AM, Varoquaux wrote:

In lib/matplotlib/axes/_axes.py:

  •    To automatically generate the legend from labels::
    
  •        line, = ax.plot([1, 2, 3], label='Inline label')
    

Ah, nice. Thanks

It's a common idiom, and I use it, but with hesitation; it "looks wrong"
at first glance, as you noticed. The alternative, explicitly indexing
with [0] at the end, is less concise but more visually obvious.
Therefore it might be preferable in documentation intended for newcomers.

Copy link
Member

Choose a reason for hiding this comment

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

I rather like the the idiom. I would suggest adding a note explaining it before changing it to [0].

@mdboom
Copy link
Member

mdboom commented Sep 20, 2013

I agree with this in concept -- I probably won't have a chance to review it properly until after tagging 1.3.1, however.

same height, set to ``[0.5]``. Default ``[0.375, 0.5, 0.3125]``.

markerscale : None or int or float
The relative size of legend markers compared with the originally
Copy link
Member

Choose a reason for hiding this comment

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

Spelling mistake? I think there's only one l in "originally".

Copy link
Member Author

Choose a reason for hiding this comment

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

It is double "l" in English. I couldn't find whether there is an American spelling (http://www.merriam-webster.com/dictionary/originally)

@NelleV
Copy link
Member

NelleV commented Sep 20, 2013

This is nice work! The documentation is much better. Thanks!

@pelson
Copy link
Member Author

pelson commented Sep 23, 2013

Thanks for all of the comments so far. Because 1ce72ff is such a big commit, I'm not going to modify that, but will add commits on top only.

I think I've now addressed @NelleV's excellent review comments (thanks for the spelling hints 馃槃).

@pelson
Copy link
Member Author

pelson commented Sep 25, 2013

Pinging @leejjoon.

Just a reminder, I'd like to merge this in 8 days (it is an arbitrary deadline, and I'd be happy enough to extend to another fixed date if requested). I think I've addressed @NelleV's excellent comments.

the first two argument of the legend call.::
Not all handles can be turned into legend entries automatically,
so it is often necessary to create an artist which *can*. Legend handles
don't have to exists on the Figure or Axes in order to be used.
Copy link
Member

Choose a reason for hiding this comment

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

typo: exists -> exist

@pelson
Copy link
Member Author

pelson commented Oct 28, 2013

I've been a little busy lately and not been able to bang the drum for this PR - sorry for letting it slip.

I've rebased, and believe I've addressed the comments above. Lets extend the merge date to this Friday - the 1st of November.

@pelson
Copy link
Member Author

pelson commented Nov 4, 2013

@mdboom - any ideas on what's up with the tests?

======================================================================
ERROR: Failure: AttributeError ('module' object has no attribute 'test_mlab')
----------------------------------------------------------------------
Traceback (most recent call last):
  File "/home/travis/virtualenv/python2.7/local/lib/python2.7/site-packages/nose/loader.py", line 402, in loadTestsFromName
    module = resolve_name(addr.module)
  File "/home/travis/virtualenv/python2.7/local/lib/python2.7/site-packages/nose/util.py", line 321, in resolve_name
    obj = getattr(obj, part)
AttributeError: 'module' object has no attribute 'test_mlab'

As far as I'm aware, I've not done anything to upset mlab, but you can never be certain I suppose.

@mdboom
Copy link
Member

mdboom commented Nov 4, 2013

Hmm... not sure. I've restarted the tests just for good measure... if still failing, I'll investigate further.

@mdboom
Copy link
Member

mdboom commented Nov 11, 2013

I'm stumped by the test failures (and can't reproduce them here). Maybe we try reverting the change to test_mlab.py and see if that makes any difference (I say, grasping at straws).

@tacaswell
Copy link
Member

This looks vaguely like the failures that are showing up on #2236 in the 3.x builds (in that there are errors that the tests don't exist).

@tacaswell
Copy link
Member

@pelson any progress on this?

@pelson
Copy link
Member Author

pelson commented Jan 6, 2014

I've rebased - the failing test wasn't to do with this change as far as I can see. Hopefully the tests will work now.

@pelson
Copy link
Member Author

pelson commented Jan 22, 2014

I've just rebased and I think this is good to go. Anyone willing to merge?

internals are revised to support
The :meth:`~matplotlib.axes.Axes.get_legend_handles_labels` function returns
a list of handles/artists which exist on the Axes which can be used to
generate entries for the resulting legend.
Copy link
Member

Choose a reason for hiding this comment

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

There should be a sentence or clause here hedging that this will not work for all artists.

Copy link
Member Author

Choose a reason for hiding this comment

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

a list of handles/artists which exist on the Axes which can be used to generate entries for the resulting legend.

There is no hedging needed. The function always works, it just doesn't always return all of the artists that have been added to the axes. I do go on to mention that not all artists can be drawn in a legend, and ideally would keep the two paragraphs separate.

Do you still want me to do this?

Copy link
Member

Choose a reason for hiding this comment

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

I don't think it is clear that it will only return the artists which have registered handlers. I want to put a reference to that other paragraph here so people realize that they may need to keep reading.

Copy link
Member Author

Choose a reason for hiding this comment

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

Ok. I'm on it.

The loc itslef can be a 2-tuple giving x,y of the lower-left corner of
the legend in axes coords (*bbox_to_anchor* is ignored).
Specific lines can be excluded from the automatic legend element
selection by defining a label starting with an underscore.
Copy link
Member

Choose a reason for hiding this comment

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

doc line related to comment about _ above.

@pelson
Copy link
Member Author

pelson commented Jan 27, 2014

Ok, I've added a commit on top of ceda68f - thanks for your thorough review @tacaswell.

@pelson
Copy link
Member Author

pelson commented Jan 28, 2014

Note - this doesn't merge cleanly (whats new perhaps) - so rather than re-base and force you to have to re-read everything, I will merge this manually once I've got your 馃憤 @tacaswell.

@tacaswell
Copy link
Member

I left one minor comment, but other than that 馃憤

Should probably change the note in api_change.rst to indicate the deprecation rather than change of the handler spec.

@pelson
Copy link
Member Author

pelson commented Jan 28, 2014

I decided to rebase @tacaswell - do you want to merge?

tacaswell added a commit that referenced this pull request Jan 28, 2014
Rewrite of the entire legend documentation, including tidy ups of code and style to all things "legend".
@tacaswell tacaswell merged commit 9e0be95 into matplotlib:master Jan 28, 2014
@pelson
Copy link
Member Author

pelson commented Jan 29, 2014

Woo-ho - thanks @tacaswell - hopefully I will stop being asked all the time how to do legends 馃槃!

@pelson pelson deleted the legend_tidy branch January 29, 2014 10:26
@tacaswell
Copy link
Member

Unlikely. Legends have a lot of moving parts.

tacaswell added a commit to tacaswell/matplotlib that referenced this pull request Jan 10, 2015
tacaswell added a commit to tacaswell/matplotlib that referenced this pull request Jan 10, 2015
tacaswell added a commit to tacaswell/matplotlib that referenced this pull request Jan 10, 2015
tacaswell added a commit to tacaswell/matplotlib that referenced this pull request Jan 11, 2015
tacaswell added a commit to tacaswell/matplotlib that referenced this pull request Jan 22, 2015
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.

None yet

7 participants