[MRG] Allow kwarg handles and labels figure.legend and make doc for kwargs the same #9324

jklymak · 2017-10-08T19:30:46Z

PR Summary

Allows fig.legend(handles=hs, labels=labs) keyword calls.

PR Checklist

Has Pytest style unit tests
Code is PEP 8 compliant
Check reworked docs are correct...

dstansby · 2017-10-08T19:50:17Z

Thanks for fixing this. I can't think of any reason not to just have them as kwargs with defaults of None as you said in the bug report, but maybe I'm missing something.

tacaswell · 2017-10-08T19:50:32Z

lib/matplotlib/figure.py

+                    warnings.warn('The handle {!r} has a label of {!r} which '
+                                  'cannot be automatically added to the '
+                                  'legend.'.format(handle, label))
+                    labels.remove(label)


Can we let this be handled down stream?

The underscore check? Maybe. I just shamelessly copied from axes.legend.

Yes yes I know I should have refactored it... otoh I don’t like that axes.legend if there is only one argument assumes that it is a list of labels. I think a list of handles makes much more sense.

Put this check into legend.Legend.__init__, and removed from axes.legend and figure.legend

Taking labels (not handles) is a bit problematic and leads to issues like benlaken/European_wind#1

@tacaswell. Um yeah that does look to be a bad decision.

I’d suggest we just strip the illegal underscore and warn (or logging.warning 😉) or set it to an empty string.

But I didn’t originate this code. I’m not actually sure why an underscore is a problem in the first place.

tacaswell · 2017-10-08T19:52:02Z

lib/matplotlib/tests/test_legend.py

@@ -74,6 +74,56 @@ def test_labels_first():
    ax.legend(loc=0, markerfirst=False)


+@image_comparison(baseline_images=['legend_fig_noargs'], extensions=['png'],


Is there a reasonable way to check that this works without an image test?

Happy to try that. Or reduce the tests. I don’t know how you would test it without a plot but that’s just my ignorance. Happy to implement something

You only need to test equivalence between the results of different invocations, not equivalence to some human-verified gold standard output.

see https://github.com/matplotlib/matplotlib/pull/9305/files#diff-663c17d0729218d60dbb862c286a17d4R5327

OK< I think I understand. Use the trick linked above and then assert that the pngs are the same w/o actually having to save them or do the image comparison? That sounds great.

Sorry for being slow, but I don't know anything about writing tests for python...

And I'm not an expert at testing with matplotlib, but I would've thought there might be a way to check the calculated legend artists are equivalent. But perhaps not.

ax.legend_.get_texts() and ax.legend_.legendHandles seems to get you back the labels and handles used, but that is just from poking at a legend object interactively....

jklymak · 2017-10-08T21:29:37Z

@dstansby Yes, I'm still not sure. But I largely copied from axes.legend and its done with *args, *kwargs as well, so I did that here.

jklymak · 2017-10-09T15:31:31Z

OK, I refactored. Now figure.legend and axes.legend have exactly the same argument parsing (now in legend._parse_legend_args).

Probably need the docs for figure.legend changed. Its possible this is non-backwards compatible. figure.legend used to allow three positional arguments, with the third being loc, but I don't think axes.legend ever did.

Question: Is there a way for figure.legend and axes.legend to share part of their docs since the calls are the same now?

This is not passing a legend test that I don't understand so I'm having difficulty debugging...

def test_legend_handler_map(self):
        lines = plt.plot(range(10), label='hello world')
        with mock.patch('matplotlib.axes.Axes.'
                        'get_legend_handles_labels') as handles_labels:
            handles_labels.return_value = lines, ['hello world']
            plt.legend(handler_map={'1': 2})
        handles_labels.assert_called_with({'1': 2})

jklymak · 2017-10-09T16:58:18Z

It seems that both instances of legend should have pass-through kwarg documentation.

It seems the right way to do this is for each keyword argument to have a set_arg that has an ACCEPTS: line in the doc. But I'm not doing that. So I assume I can define a kw doc string and then reference it somehow.

Ah, I knew I'd seen it somwhere. colorbar_doc is a great example.

tacaswell · 2017-10-09T23:53:30Z

Well, that escalated quickly. I thought this was going to be a ~5 line fix....

anntzer · 2017-10-10T00:29:42Z

lib/matplotlib/legend.py

@@ -105,6 +106,177 @@ def _update_bbox_to_anchor(self, loc_in_canvas):
        self.legend.set_bbox_to_anchor(loc_in_bbox)


+legend_kw_doc = '''


make this _private?

jklymak · 2017-10-10T03:50:43Z

@tacaswell We could easily go back to the 5-line fix...

But the fundamental problem was that *args, **kwargs were being handled differently between figure.legend and axes.legend for no good reason, and it will just be a maintenance headache in the future. Worse the legend.Legend keywords were described three different places in three different formats, inconsistently in each place.

The changes are actually pretty small.

moved the argument-parsing logic out of axes.legend into legend._parse_legend_args
used the same parsing in figure.legend
changed the three docstrings to refer to legend_kw_doc.

I need to do some cleanup, and I still have no idea how to address the failing mock test (see comment above), but otherwise I hope this is an improvement.

jklymak · 2017-10-11T02:26:05Z

Oh, hmmmm. fig.legend() did this funky thing where if you called

fig, axs = plt.subplots(2,1)
l1 = axs[0].plot(x, 'r', label='boo')
l2 = axs[1].plot(x, 'r', label='boo')
fig.legend()

It would only put 'boo' in the legend once. This PR doesn't respect this. Easy to fix, but do we really want to?

anntzer · 2017-10-11T02:32:49Z

I guess we "have to" for backcompatibility but that sounds like yet another thing I'd like to get rid of... (for example Axes.legend does not provide the same functionality).

jklymak · 2017-10-11T02:39:40Z

@anntzer Ahem, now it does ;-) Well, if this PR gets accepted. I actually think there is some sense to it, but I wouldn't have offered the functionality if it'd just have been up to me...

anntzer · 2017-10-11T02:54:39Z

I can't say I really hate this feature (so if others like it I'm fine with it), but in my view it's a bit similar to auto reuse of axes (#9037 and linked isses) and would prefer not having it for the same reason.

jklymak · 2017-10-11T03:49:14Z

lib/matplotlib/legend.py

+    handles = []
+    labels = []
+
+    def _in_handles(h, l):


@anntzer This is where the logic to remove duplicate label/line pairs is. Easy to remove if there is a consensus.

tacaswell · 2017-10-11T03:50:18Z

lib/matplotlib/figure.py

-                            '(artist handles, figure labels, legend location)')
-
-        l = Legend(self, handles, labels, **kwargs)
+        print("Axes:", self.axes)


rouge print

tacaswell · 2017-10-11T03:58:51Z

lib/matplotlib/legend.py

+
+    # Two arguments:
+    #   * user defined handles and labels
+    elif len(args) >= 2:


This should be ==, passing loc as a positional argument in deprecated for the Axes version but apparently we missed doing the same to the Figure version (as it did not explicitly take loc as a positional, but passed *args through to the Legend.__init__ so it worked.

This should probably return handles, labels, extra_args and in Axes.legend raise if len(extra_args) > 0 and pass them through in Figure.legend

OK, this is caught in lib/matplotlib/axes/_axes.py. But it still needs to be in here.

tacaswell · 2017-10-11T04:04:17Z

lib/matplotlib/axes/_axes.py

+        kwargs.pop('handles', None)
+        kwargs.pop('labels', None)
+        # check for third arg
+        if len(args) == 3:


This was intentionally deprecated in 1.5 http://matplotlib.org/api/api_changes.html#legend it should not come back!

tacaswell · 2017-10-11T04:05:13Z

Modulo the handling of a positional loc being inconsistent between axes and figure 👍

tacaswell

Raise on positional loc in Axes.legend, still pass it through on Figure.legend (but we should deprecate that on master once this lands).

jklymak · 2017-10-11T04:08:53Z

@tacaswell Can we just deprecate the third positional argument in both? Right now the code doesn't care if its called from figure or axes.

That said, I'm not at all arguing for this third positional argument.

tacaswell · 2017-10-11T04:28:49Z

In the spirit of minimal change / semver I would rather deprecate loc on Figure.legend in 2.2 rather than 2.1.1. It is already removed from Axes.legend from 1.5 so we don't need to deprecate it there ;)

jklymak · 2017-10-11T04:36:28Z

@tacaswell Allowing the third positional argument is back in Axes.legend with this PR, because all the code to handle the arguments is the now same between Axes.legend and figure.legend. Thats how this PR got rather large-looking. If we handle figure and Axes differently, their code bases will diverge again, and thats how we ended up in this mess in the first place!

I'm suggesting we silently go back to allowing the third positional argument in Axes.legend, but deprecate if anyone happens to use it, and deprecate it in figure.legend as well. Then we can take it out again for 2.2. Its not like it hurts anything to handle the third positional argument.

tacaswell · 2017-10-11T04:46:24Z

A user will discover it and complain when it goes away. I the current behavior can be maintained on top of this refactor without too much extra complexity.

In Axes.legend()

def legend(self, *args, **kwargs):
    handles, labels, extra_args = _parse_legend_args([self], *args, **kwargs)
    if len(extra_args):
        raise TypeError(....)
    l = legend.Legend(handles, labels, **kwargs)
    ...

and in Figure.legend()

def legend(self, *args, **kwargs):
    handles, labels, extra_args = _parse_legend_args(...)
    l = legend.Legend(handles, labels, *extra_args, **kwargs)
    ...

and in _parse_legend_args

extra_args = ()
if ...:
    ...
elif len(args) >= 2:
    handles, labels = args[:2]
    extra_args = args[2:]

return handles, labels, extra_args

The current behavior of Figure.legend is that all inputs to legend.Legend can be passed as positional so this is a bit worse than I initially thought.

jklymak · 2017-10-11T04:50:42Z

Ah OK, I understand. Thats easy enough, if a bit of a PITA.

Not sure I understand your last comment, though. Currently only handles, labels, and loc can be passed as positional, and loc actually gets converted to a kwarg.

tacaswell · 2017-10-11T05:56:25Z

In 2.0 the truncated Figure.legend is

def legend(self, handles, labels, *args, **kwargs):
    l = Legend(self, handles, labels, *args, **kwargs)
    self.legends.append(l)
    l._remove_method = lambda h: self.legends.remove(h)
    self.stale = True
    return l

so the API change from 2.0 -> 2.1 is bigger than just breaking handlers and labels as kwargs. If we are going to go back to the < 2.1 behavior we should bring it all back, add deprecation warnings to positional args in 2.2 and then remove in the next release.

tacaswell · 2017-10-11T05:56:44Z

Sorry, I was using confusing tense!

jklymak · 2017-10-23T20:26:02Z

Believe it or not, ax.legend(arg1) can also accept just a list of handles, a fact I found out only because @choldgraf did it in http://matplotlib.org/devdocs/gallery/text_labels_and_annotations/custom_legends.html#sphx-glr-gallery-text-labels-and-annotations-custom-legends-py

But, there is no way to attach a proper label to the legend when you call the legend like this, even if the underlying handles have labels! In master:

N = 10
data = [np.logspace(0, 1, 100) + np.random.randn(100) + ii for ii in range(N)]
data = np.array(data).T
cmap = plt.cm.coolwarm
rcParams['axes.prop_cycle'] = cycler(color=cmap(np.linspace(0, 1, N)))

fig, ax = plt.subplots()
lines=[]
for i in range(N):
    lines += [ax.plot(data[:, i], label='%s'%i)]
    print(lines[-1])
ax.legend(lines)

That is broken beyond belief in my opinion, because:

fig, ax = plt.subplots()
lines=[]
for i in range(N):
    lines += [ax.plot(data[:, i], label='%s'%i)]
    print(lines[-1])
ax.legend(lines[::3])

is a completely reasonable thing to do, but yields:

I won't fix in this PR, but wanted to register my dismay.

I think ax.legend(arg) should accept either a list fo strings (old label format), or a list of handles. If the first, it does what it does now and just finds the first N handles to attach the strings to. If the latter, it provides a legend for that list.

The arg = list of handles is not documented, and I think @choldgraf example above should be modified because it is doing something legend isn't really meant to do (all tests and docs passed for my change before this example was put into master). I think we should add support for lists of handles, but only display them if the label is explicitly set, and document this as new behaviour. If you'd like that as a 2.2 separate PR, thats fine. I think the PR here, as it stands, gives us back what we had before 2.1. Pinging @tacaswell @anntzer @dstansby Thanks!

anntzer · 2017-10-23T20:53:21Z

As your second example shows, ax.legend is not "accepting a list of handles" (you will note that the artists that went into the legend in the second example are not every third, but the first 4); instead, it is interpreting the (positional first) argument as a list of labels (after forcefully converting them using str) and assigning them as labels to the first n artists.

Yes this is nonsensical, but should really be the object of a separate PR.

jklymak · 2017-10-24T22:18:34Z

This is ready to go, unless we find something else whacky that the various versions of legend do...

I have two approvals, but I assume its bad form to self-merge?

tacaswell · 2017-10-30T19:50:43Z

@jklymak I think we did something funny with git :(

jklymak · 2017-10-30T19:52:06Z

Sorry! My fault. Let me re-push. I made a local change and was pushing onto your change. But I got your change. I won't touch it until tonight, honest!

dopplershift

I'm not wild about such a big PR for a patch release, but I get why.

I don't like the doc handling, but that's a bit of a mess in matplotlib throughout, so this is up to par. Let's save the big docstring handling throw down for #9414.

jklymak · 2017-10-30T19:56:29Z

@dopplershift Yes fair enough - its easy to fix the docs later. I'll add a note that this is one place to fix in #9414 if its not there already....

jklymak · 2017-10-31T00:35:30Z

lib/matplotlib/figure.py

+            #     "instead.")
+            # kwargs['loc'] = extra_args[0]
+            # extra_args = extra_args[1:]
+            pass


@tacaswell This now fails Travis - what was the plan here? I thought we were keeping figure.legend w/ three possible args?

This shouldn't have changed anything other than to prevent fig.legend(a, b, l, loc=c) case.

Ah, I see:

self = <matplotlib.tests.test_legend.TestLegendFigureFunction object at 0x7fffd6b1fc50> def test_legend_label_three_args(self): fig, ax = plt.subplots() lines = ax.plot(range(10)) with mock.patch('matplotlib.legend.Legend') as Legend: fig.legend(lines, ['foobar'], 'right') > Legend.assert_called_with(fig, lines, ['foobar'], loc='right')

The test was checking that loc got loaded w/ the third element. I'll fix later. Thanks!

@tacaswell Changed legend_test to check for an Exception should someone call: fig.legend(handles, labels, 'right', loc='left') (test_legend_label_three_args_pluskw) I think thats what you wanted.

Any reason to leave this check/pass in here? Just posterity sake?

To avoid changing the API as much as possible. I am 👍 on deprecating the other positional arguments for 2.2, but not in 2.1.1 (so leave that code in place so we can just un-comment it in a follow on PR).

I don't think we needed a test for what is core-python behavior (you can not pass a value as both positional and keyword) nor should we break it!

Allow kwarg handles and labels figure.legend Small refactor of check for labels so it is inside legend.Legend attempt to refactor legend; not passing tests small typo Moved kwarg documentation out of child functions and into legend. Fixed documentation passthroughs so they work properly Fixed documentation passthroughs so they work properly Remerge master, small changes Fixed tests Remove repeated labels if same linestyle Remove repeated labels if same linestyle Removed ability of third positional argument for axes.legend Deprecate third psotionsal argument for fig.legend Allow parasite axes to call legend(): parasite_simple.py Fixed doc Fixed doc Added twinx test for legend merge fix merge fix Fixed legend handling of handles-only input; added error message Fixed legend handling of handles-only input for py27 Fixed small doc change MNT: do not special-case loc as positional arg Fixed small doc change Fix PEP8 and test error Fix PEP8 and test error

… and make doc for kwargs the same

jklymak · 2017-10-31T17:52:32Z

Ummm, help! The original example no longer works!!

fig, ax = plt.subplots(1, 2)
hs = []
lab = ['Boo', 'Hoo']
for nn, axx in enumerate(ax):
    h, = axx.plot(np.arange(10))
    hs += [h]
fig.legend(hs, lab)
plt.show()


/Users/jklymak/matplotlib/lib/matplotlib/legend.py:932: UserWarning: Legend does not support [<matplotlib.lines.Line2D object at 0x107cfc3c8>] instances.
A proxy artist may be used instead.
See: http://matplotlib.org/users/legend_guide.html#creating-artists-specifically-for-adding-to-the-legend-aka-proxy-artists
  "aka-proxy-artists".format(orig_handle)
/Users/jklymak/matplotlib/lib/matplotlib/legend.py:932: UserWarning: Legend does not support [<matplotlib.lines.Line2D object at 0x107cfc908>] instances.
A proxy artist may be used instead.
See: http://matplotlib.org/users/legend_guide.html#creating-artists-specifically-for-adding-to-the-legend-aka-proxy-artists
  "aka-proxy-artists".format(orig_handle)

jklymak · 2017-10-31T17:56:05Z

Ooops, false alarm. I had a typo in my test!

import numpy as np
import matplotlib.pyplot as plt

fig, ax = plt.subplots(1, 2)
hs = []
for axx in ax:
    h, = axx.plot(np.arange(10))
    hs += [h]
fig.legend(handles=hs, labels=['Boo', 'Hoo'])
plt.show()

works fine!

Backport PR #9324 on branch v2.1.x

tacaswell reviewed Oct 8, 2017

View reviewed changes

dstansby added the Release critical For bugs that make the library unusable (segfaults, incorrect plots, etc) and major regressions. label Oct 8, 2017

dstansby added this to the 2.1.1 (next bug fix release) milestone Oct 8, 2017

tacaswell reviewed Oct 8, 2017

View reviewed changes

jklymak changed the title ~~Allow kwarg handles and labels figure.legend~~ WIP: Allow kwarg handles and labels figure.legend and make doc for kwargs the same Oct 9, 2017

anntzer reviewed Oct 10, 2017

View reviewed changes

jklymak mentioned this pull request Oct 10, 2017

2.1 figure.legend broken #9320

Closed

jklymak commented Oct 11, 2017

View reviewed changes

tacaswell reviewed Oct 11, 2017

View reviewed changes

tacaswell requested changes Oct 11, 2017

View reviewed changes

jklymak force-pushed the fixfiglegend branch from 79ea23e to 6516c1c Compare October 24, 2017 03:41

jklymak changed the title ~~Allow kwarg handles and labels figure.legend and make doc for kwargs the same~~ [MRG] Allow kwarg handles and labels figure.legend and make doc for kwargs the same Oct 24, 2017

jklymak mentioned this pull request Oct 24, 2017

Stem performance boost #9565

Closed

jklymak force-pushed the fixfiglegend branch from c63b780 to 1cbf4f7 Compare October 30, 2017 19:52

dopplershift approved these changes Oct 30, 2017

View reviewed changes

jklymak force-pushed the fixfiglegend branch from 1cbf4f7 to 4bc1efd Compare October 30, 2017 21:26

jklymak commented Oct 31, 2017

View reviewed changes

jklymak force-pushed the fixfiglegend branch from 4bc1efd to 5374de5 Compare October 31, 2017 03:58

jklymak force-pushed the fixfiglegend branch from 5374de5 to 2b6927d Compare October 31, 2017 04:12

tacaswell merged commit d2a8e67 into matplotlib:master Oct 31, 2017

lumberbot-app bot mentioned this pull request Oct 31, 2017

Backport PR #9324 on branch v2.1.x #9644

Merged

lumberbot-app bot pushed a commit that referenced this pull request Oct 31, 2017

Backport PR #9324: [MRG] Allow kwarg handles and labels figure.legend…

4f594f4

… and make doc for kwargs the same

jklymak deleted the fixfiglegend branch October 31, 2017 19:44

QuLogic added a commit that referenced this pull request Nov 3, 2017

Merge pull request #9644 from matplotlib/auto-backport-of-pr-9324

24ba7a8

Backport PR #9324 on branch v2.1.x

jksuom mentioned this pull request Dec 12, 2017

sympy/plotting/tests/test_plot.py matplotlib failures on Travis sympy/sympy#13730

Closed

This was referenced Dec 14, 2017

improve legend docstring #9999

Merged

Only one legend entry is rendered for items with the same label and color #10056

Closed

FIX: remove repeated label legend logic #10064

Merged

jklymak mentioned this pull request Mar 15, 2018

DOC: make legend docstring interpolated #10799

Merged

2 tasks

jklymak mentioned this pull request Jul 17, 2018

Use super() instead of manually fetching supermethods for parasite axes. #11678

Merged

6 tasks

Jacob-Stevens-Haas mentioned this pull request Oct 20, 2023

[ENH]: Make "No artists with labels found to put in legend" a warning #27145

Closed

		@@ -74,6 +74,56 @@ def test_labels_first():
		ax.legend(loc=0, markerfirst=False)


		@image_comparison(baseline_images=['legend_fig_noargs'], extensions=['png'],

		@@ -105,6 +106,177 @@ def _update_bbox_to_anchor(self, loc_in_canvas):
		self.legend.set_bbox_to_anchor(loc_in_bbox)


		legend_kw_doc = '''

[MRG] Allow kwarg handles and labels figure.legend and make doc for kwargs the same #9324

[MRG] Allow kwarg handles and labels figure.legend and make doc for kwargs the same #9324

Conversation

jklymak commented Oct 8, 2017 • edited Loading

PR Summary

PR Checklist

dstansby commented Oct 8, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jklymak commented Oct 8, 2017

jklymak commented Oct 9, 2017 • edited Loading

jklymak commented Oct 9, 2017 • edited Loading

tacaswell commented Oct 9, 2017

Choose a reason for hiding this comment

jklymak commented Oct 10, 2017

jklymak commented Oct 11, 2017

anntzer commented Oct 11, 2017 • edited Loading

jklymak commented Oct 11, 2017 • edited Loading

anntzer commented Oct 11, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tacaswell commented Oct 11, 2017

tacaswell left a comment

Choose a reason for hiding this comment

jklymak commented Oct 11, 2017 • edited Loading

tacaswell commented Oct 11, 2017

jklymak commented Oct 11, 2017

tacaswell commented Oct 11, 2017

jklymak commented Oct 11, 2017

tacaswell commented Oct 11, 2017

tacaswell commented Oct 11, 2017

jklymak commented Oct 23, 2017

anntzer commented Oct 23, 2017 • edited Loading

jklymak commented Oct 24, 2017

tacaswell commented Oct 30, 2017

jklymak commented Oct 30, 2017

dopplershift left a comment

Choose a reason for hiding this comment

jklymak commented Oct 30, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jklymak commented Oct 31, 2017

jklymak commented Oct 31, 2017

jklymak commented Oct 8, 2017 •

edited

Loading

jklymak commented Oct 9, 2017 •

edited

Loading

jklymak commented Oct 9, 2017 •

edited

Loading

anntzer commented Oct 11, 2017 •

edited

Loading

jklymak commented Oct 11, 2017 •

edited

Loading

jklymak commented Oct 11, 2017 •

edited

Loading

anntzer commented Oct 23, 2017 •

edited

Loading