Rewrite of the entire legend documentation, including tidy ups of code and style to all things "legend". by pelson · Pull Request #2442 · matplotlib/matplotlib

pelson · 2013-09-20T09:32:43Z

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!

pelson · 2013-09-20T09:34:35Z

lib/matplotlib/testing/decorators.py

@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. 😉

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

NelleV · 2013-09-20T12:40:04Z

lib/matplotlib/axes/_axes.py

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

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

Ah, nice. Thanks

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.

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

mdboom · 2013-09-20T12:43:15Z

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

NelleV · 2013-09-20T12:44:54Z

lib/matplotlib/axes/_axes.py

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

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

NelleV · 2013-09-20T13:05:49Z

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

pelson · 2013-09-23T10:01:56Z

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 · 2013-09-25T12:50:32Z

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.

mdboom · 2013-09-25T14:05:31Z

doc/users/legend_guide.rst

typo: exists -> exist

pelson · 2013-10-28T09:02:06Z

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 · 2013-11-04T10:28:59Z

@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 · 2013-11-04T17:59:04Z

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

mdboom · 2013-11-11T17:51:10Z

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 · 2013-11-11T18:40:38Z

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 · 2014-01-05T23:04:27Z

@pelson any progress on this?

pelson · 2014-01-06T09:47:42Z

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 · 2014-01-22T15:37:06Z

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

tacaswell · 2014-01-22T15:46:34Z

doc/users/legend_guide.rst

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

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?

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.

Ok. I'm on it.

tacaswell · 2014-01-22T16:13:05Z

lib/matplotlib/axes/_axes.py

doc line related to comment about _ above.

pelson · 2014-01-27T18:06:45Z

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

pelson · 2014-01-28T09:55:06Z

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 · 2014-01-28T12:59:06Z

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.

and style to all things "legend".

pelson · 2014-01-28T14:27:51Z

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

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

pelson · 2014-01-29T10:26:15Z

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

tacaswell · 2014-01-29T13:22:28Z

Unlikely. Legends have a lot of moving parts.

Deprecated in matplotlib#2442 / 9e0be95

pelson reviewed Sep 20, 2013
View reviewed changes

pelson mentioned this pull request Sep 20, 2013

Implement LegendHandler for FeatureArtist SciTools/cartopy#334

Open

NelleV reviewed Sep 20, 2013
View reviewed changes

mdboom reviewed Sep 25, 2013
View reviewed changes

doc/users/legend_guide.rst Outdated

Copy link
Copy Markdown

Member

mdboom Sep 25, 2013

Choose a reason for hiding this comment

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

typo: exists -> exist

pelson mentioned this pull request Nov 8, 2013

Fix typo in legend documentation #2584

Merged

tacaswell reviewed Jan 22, 2014
View reviewed changes

lib/matplotlib/axes/_axes.py

Copy link
Copy Markdown

Member

tacaswell Jan 22, 2014

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 added 2 commits January 28, 2014 13:56

Rewrite of the entire legend documentation, including tidy ups of code

99f325d

and style to all things "legend".

Review actions.

59e7d13

tacaswell added a commit that referenced this pull request Jan 28, 2014

Merge pull request #2442 from pelson/legend_tidy

9e0be95

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

tacaswell merged commit 9e0be95 into matplotlib:master Jan 28, 2014

pelson deleted the legend_tidy branch January 29, 2014 10:26

tacaswell added a commit to tacaswell/matplotlib that referenced this pull request Jan 10, 2015

MNT : remove code to handle callable legend handlers

342dde1

Deprecated in matplotlib#2442 / 9e0be95

tacaswell added a commit to tacaswell/matplotlib that referenced this pull request Jan 10, 2015

MNT : remove code to handle callable legend handlers

84f7a51

Deprecated in matplotlib#2442 / 9e0be95

tacaswell added a commit to tacaswell/matplotlib that referenced this pull request Jan 10, 2015

MNT : remove code to handle callable legend handlers

c43107e

Deprecated in matplotlib#2442 / 9e0be95

tacaswell added a commit to tacaswell/matplotlib that referenced this pull request Jan 11, 2015

MNT : remove code to handle callable legend handlers

8dfd02a

Deprecated in matplotlib#2442 / 9e0be95

tacaswell added a commit to tacaswell/matplotlib that referenced this pull request Jan 22, 2015

MNT : remove code to handle callable legend handlers

d17863f

Deprecated in matplotlib#2442 / 9e0be95

ImportanceOfBeingErnest mentioned this pull request Nov 26, 2019

DOCS 2D Line label option _nolegend_ is not documented #15770

Closed

anntzer mentioned this pull request Jun 26, 2023

FIX: behaviour of legend labels with leading underscores did not adhere to the API documentation #24713

Closed

2 tasks

Uh oh!

Conversation

pelson commented Sep 20, 2013

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mdboom commented Sep 20, 2013

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

NelleV commented Sep 20, 2013

Uh oh!

pelson commented Sep 23, 2013

Uh oh!

pelson commented Sep 25, 2013

Uh oh!

Choose a reason for hiding this comment

Uh oh!

pelson commented Oct 28, 2013

Uh oh!

pelson commented Nov 4, 2013

Uh oh!

mdboom commented Nov 4, 2013

Uh oh!

mdboom commented Nov 11, 2013

Uh oh!

tacaswell commented Nov 11, 2013

Uh oh!

tacaswell commented Jan 5, 2014

Uh oh!

pelson commented Jan 6, 2014

Uh oh!

pelson commented Jan 22, 2014

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

pelson commented Jan 27, 2014

Uh oh!

pelson commented Jan 28, 2014

Uh oh!

tacaswell commented Jan 28, 2014

Uh oh!

pelson commented Jan 28, 2014

Uh oh!

pelson commented Jan 29, 2014

Uh oh!

tacaswell commented Jan 29, 2014

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants