Allow figure.legend to be called without arguments by dstansby · Pull Request #7811 · matplotlib/matplotlib

dstansby · 2017-01-12T21:06:30Z

This supersedes #2128.

The actual behind-the-scenes code is the same as figure.legend can be called without arguments #2128
Examples updated for current freetype, and rebased onto current (ish) master
I've cleaned up the docstring

(p.s. I can squash this once it's all been reviewed)

codecov-io · 2017-01-12T23:19:15Z

Current coverage is 62.17% (diff: 73.33%)

Merging #7811 into master will increase coverage by 0.08%

@@             master      #7811   diff @@
==========================================
  Files           174        174          
  Lines         56050      56119    +69   
  Methods           0          0          
  Messages          0          0          
  Branches          0          0          
==========================================
+ Hits          34806      34894    +88   
+ Misses        21244      21225    -19   
  Partials          0          0

Powered by Codecov. Last update 70bb531...8a78947

NelleV

I've added some minor comments, but it looks good overall. There is just a what's new item missing.

It'd also be great if we could improve the error message.

NelleV · 2017-01-13T00:44:02Z

@@ -1,19 +1,19 @@
 import numpy as np


While you are at it, do you mind adding a docstring to give this example a title and explain what it does? See http://matplotlib.org/devdocs/devel/MEP/MEP12.html for examples.

NelleV · 2017-01-13T00:44:12Z

-fig = plt.figure()
-ax1 = fig.add_axes([0.1, 0.1, 0.4, 0.7])
-ax2 = fig.add_axes([0.55, 0.1, 0.4, 0.7])
+fig, axs = plt.subplots(1, 2)


NelleV · 2017-01-13T00:44:59Z

        return self.axes

-    def legend(self, handles, labels, *args, **kwargs):
+    def legend(self, *args, **kwargs):


While you are at it, do you mind migrating this to a numpydoc compatible docstring?
(I can also do it if you don't mind me pushing to your branch).

NelleV · 2017-01-13T00:47:14Z

+            kwargs['loc'] = loc
+
+        else:
+            raise TypeError('Invalid arguments to legend')


Do you think we could have an error message more specific? It's quite obscure right now.
I like better "Invalide number of arguments to legend. Possible arguments are (etc)"

NelleV · 2017-01-13T00:50:36Z

And thanks for the patch :)

dstansby · 2017-01-14T20:38:14Z

Hold on a minute, not quite done! (sorry for the early push)

dstansby · 2017-01-14T20:46:29Z

Okay, that should be everything (I think!)

QuLogic · 2017-01-14T20:35:32Z

+                'upper left'      2
+                'lower left'      3
+                'lower right'     4
+                'right'           5


I wonder why there's this seemingly extra location.

I think it's the same in axes.legend() too.

QuLogic · 2017-01-14T20:39:03Z

+            ``legend.handlelength`` :data:`rcParam<matplotlib.rcParams>`.
+
+        handletextpad : float or None
+            The pad between the legend handle and text.


pad -> padding

QuLogic · 2017-01-14T20:39:26Z

-        parameters. The dimensions of these values are given as a fraction
-        of the fontsize. Values from rcParams will be used if None.
+        borderaxespad : float or None
+            The pad between the axes and legend border.


pad -> padding

QuLogic · 2017-01-14T20:42:30Z

+
+        borderpad : float or None
+            The fractional whitespace inside the legend border.
+            Measured in font-size units.


I would add this on to the end of the previous sentence, and do the same in all the following parameters: The fractional whitespace inside the legend border, measured in font-size units.

This and the 2 comments below are on bits of docstring directly copied from axes.legend(), so should probably fix them there at some point too.

QuLogic · 2017-01-14T21:04:28Z

+    fig, axes = plt.subplots(2)
+    axes[0].plot([0, 1], [1, 0], label='x', color='g')
+    axes[0].plot([0, 1], [0, 1], label='y', color='r')
+    axes[1].plot([0, 1], [1, 0], label='y', color='r')


What happens when the labels are the same, but the style is different? I believe it should be separate, but it's not tested.

Yep, seems fine, added it in the image test

tacaswell · 2017-01-15T01:35:48Z

+                ===============   =============
+                Location String   Location Code
+                ===============   =============
+                'best'            0 (currently not supported for figure legends)


Just omit this line if it is not supported?

dstansby · 2017-01-15T10:50:37Z

Okay, there goes the un-used location code and with it the PEP8 violation. I've also squashed it all down into one commit since there were at least 2 changes to the test figures previously.

anntzer · 2017-01-18T23:00:22Z

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


It would be nice to leave a comment as to what's happening here (at least it's not obvious to me at all...).

How's that? If you can think of better wording feel free to push to my branch.

Sounds good. Typo: "consdier".
I would make it a comment rather than a docstring to save some whitespace, and get rid of the whitespace between the try... catch blocks (right now it's very spread out).

anntzer · 2017-01-19T21:59:22Z

LGTM. Someone else wants to re-review it after the (minor) changes?

NelleV · 2017-01-20T00:40:21Z

LGTM. I'll wait until appveyor's finished before merging.

jnothman · 2017-10-09T23:41:26Z

This appears to break invocations like figure.legend(artists, labels=labels), since it assumes labels will be specified as a positional arg, not named. Rendering scikit-learn docs breaks at https://github.com/scikit-learn/scikit-learn/blob/master/examples/neural_networks/plot_mlp_training_curves.py#L88 with the latest release.

NelleV requested changes Jan 13, 2017

View reviewed changes

NelleV mentioned this pull request Jan 13, 2017

figure.legend can be called without arguments #2128

Closed

NelleV approved these changes Jan 14, 2017

View reviewed changes

QuLogic reviewed Jan 14, 2017

View reviewed changes

tacaswell reviewed Jan 15, 2017

View reviewed changes

tacaswell added this to the 2.1 (next point release) milestone Jan 15, 2017

Allow figure.legend to take no arguments

12c5be9

dstansby force-pushed the figure-legend branch from d21efe0 to 12c5be9 Compare January 15, 2017 10:50

NelleV changed the title ~~Allow figure.legend to be called without arguments~~ [MRG+1] Allow figure.legend to be called without arguments Jan 16, 2017

anntzer reviewed Jan 18, 2017

View reviewed changes

dstansby added 2 commits January 19, 2017 18:51

Explanatory comment in figure.legend

35a74eb

Slim down line count (and fix typo) in figure.legend

8a78947

NelleV merged commit 9000e58 into matplotlib:master Jan 20, 2017

QuLogic changed the title ~~[MRG+1] Allow figure.legend to be called without arguments~~ Allow figure.legend to be called without arguments Jan 20, 2017

dstansby deleted the figure-legend branch January 22, 2017 13:54

jklymak mentioned this pull request Oct 8, 2017

2.1 figure.legend broken #9320

Closed

Conversation

dstansby commented Jan 12, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

codecov-io commented Jan 12, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Current coverage is 62.17% (diff: 73.33%)

Uh oh!

NelleV left a comment

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!

NelleV commented Jan 13, 2017

Uh oh!

dstansby commented Jan 14, 2017

Uh oh!

dstansby commented Jan 14, 2017

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!

QuLogic Jan 14, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

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!

dstansby commented Jan 15, 2017

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!

anntzer commented Jan 19, 2017

Uh oh!

NelleV commented Jan 20, 2017

Uh oh!

jnothman commented Oct 9, 2017

Uh oh!

Reviewers

Uh oh!

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

dstansby commented Jan 12, 2017 •

edited

Loading

codecov-io commented Jan 12, 2017 •

edited

Loading

QuLogic Jan 14, 2017 •

edited

Loading