Add legend handler and artist for FancyArrow #10688

magedhennawy · 2018-03-05T20:43:24Z

I added new legend handlers for FancyArrowPatch, Annotation artists and HandlerAnnotation.

HandlerAnnotation can draw FancyArrowPatch artist in the legend:

if the annotation has an arrow, it uses HandlerFancyArrowPatch to draw an arrow

Example code:

import matplotlib.pylab as plt
from matplotlib.patches import FancyArrowPatch
from matplotlib.legend_handler import HandlerAnnotation

fig, ax = plt.subplots(1)
ax.plot([0, 1], [0, 0], label='line1')
ax.plot([0, 1], [1, 1], label='line2')
ax.annotate("",
            xy=(0.3,1.0),
            xytext=(0.3,0.0),
            arrowprops={'arrowstyle':'<->', 'color':'C7' },
            label='distance')
ax.legend()

plt.show()

**************** Added extra features. ******************

after merge with #8292

I added new legend handlers for Text, FancyArrowPatch and Annotation artists (HandlerText, HandlerFancyArrowPatch and HandlerAnnotation).

HandlerAnnotation can draw different artists in the legend:

if the annotation has no text and no arrow, it draws a white Rectangle
if the annotation has only text, it uses HandlerText to draw a text. The text is replaced by 'Aa' if it is longer than one character. (This is configurable by setting rep_str and rep_maxlen.)
if the annotation has only an arrow, it uses HandlerFancyArrowPatch to draw an arrow
If the annotation has both text and arrow, it uses HandlerTuple to draw both.
In HandlerTuple, I added the possibility of different width ratios for the artists. HandlerAnnotation uses a width_ratios=[1,4] (ie. the arrow is 4 times wider than the text).

Example code:

import matplotlib.pylab as plt
from matplotlib.patches import FancyArrowPatch
from matplotlib.legend_handler import HandlerAnnotation

fig, ax = plt.subplots(1)
ax.plot([0, 1], [0, 0], label='line1')
ax.plot([0, 1], [1, 1], label='line2')

# no text, no arrow
ax.annotate("",
            xy=(0.1,0.5),
            xytext=(0.1,0.5),
            label='annotation (empty)')

# text, no arrow
my_annotation = ax.annotate("X",
            xy=(0.1,0.5),
            xytext=(0.1,0.5),
            color='C2',
            label='annotation (text, no arrow)')

# no text, arrow
ax.annotate("",
            xy=(0.3,1.0),
            xytext=(0.3,0.0),
            arrowprops={'arrowstyle':'<->', 'color':'C7' },
            label='annotation (no text, arrow)')

# text, arrow
ax.annotate("Some long string",
            xy=(0.4,1.0),
            xytext=(0.35,0.1),
            arrowprops={'arrowstyle':'->', 'color':'C2' },
            color='C1',
            label='annotation (text + arrow)')

# Fancy arrow patch
arrpatch = FancyArrowPatch([0.5, 0.8], [0.9, 0.9],
                           arrowstyle='<|-',
                           mutation_scale=20,
                           color='C3',
                           label='arrowpatch')
ax.add_patch(arrpatch)

# Long text, will not be used in legend
ax.text(x=0.1, y=0.1,
        s='Hello',
        color='C5',
        label='text')

# Short text, copied in legend
ax.text(x=0.1, y=0.2,
        s='Z',
        color='C0',
        label='short text')

ax.legend(handler_map={my_annotation: HandlerAnnotation(rep_str='Abcde', rep_maxlen=0)})
fig.show()

jklymak · 2018-03-05T20:58:54Z

@magedhennawy you'll need to rebase onto master to get the correct tests etc to run. Sorry, a lot has chnaged recently with the move to MPL 3.0 (python 3 only).

magedhennawy · 2018-03-05T21:06:50Z

@jklymak rebased :)

jlecoeur · 2018-03-05T21:13:08Z

@magedhennawy Thanks for taking #8292 over! I was not realistically going to complete it anytime soon.

magedhennawy · 2018-03-05T21:16:54Z

@jlecoeur no problem, thank YOU for the extra features, i had only fixed the issue, you sir went above and beyond. and made my life easy <3

jklymak

This looks really cool. Thanks so much for everyone's work on it. Some gotchyas:

Please revert all the legend doc style changes (or some folks who just made all those changes are going to be pretty sore at you).

Please provide a better docstring for width_ratios. I didn't really dig into what you are talking about, but I shouldn't have to to understand a doc string.

A more comprehensive test in the modern style would be appreciated. (also why is the arrow off the page?)

I think this should go in. It'll need a whats new entry, and ideally an example.

jklymak · 2018-03-05T22:12:33Z

lib/matplotlib/legend.py

-    Default is ``None``, which will take the value from
-    :rc:`legend.markerscale`.
+    drawn ones. Default is ``None`` which will take the value from
+    the ``legend.markerscale`` :data:`rcParam <matplotlib.rcParams>`.


:rc: is the proper markup for rc params. Please undo this.

jklymak · 2018-03-05T22:13:08Z

lib/matplotlib/legend.py

-    :rc:`legend.numpoints`.
+    entry for a line/:class:`matplotlib.lines.Line2D`.
+    Default is ``None`` which will take the value from the
+    ``legend.numpoints`` :data:`rcParam<matplotlib.rcParams>`.


please leave the :rc: markup as it was.

jklymak · 2018-03-05T22:14:10Z

lib/matplotlib/legend.py

@@ -134,14 +136,15 @@ def _update_bbox_to_anchor(self, loc_in_canvas):
    corner of the legend in axes coordinates (in which case
    ``bbox_to_anchor`` will be ignored).

-bbox_to_anchor : `.BboxBase` or pair of floats
+bbox_to_anchor : :class:`matplotlib.transforms.BboxBase` instance \


The original link to .BboxBase should have been fine as it was. Saves you having to continue the line.

jklymak · 2018-03-05T22:14:23Z

lib/matplotlib/legend.py

-    :rc:`legend.frameon`.
+    Control whether the legend should be drawn on a patch (frame).
+    Default is ``None`` which will take the value from the
+    ``legend.frameon`` :data:`rcParam<matplotlib.rcParams>`.


jklymak · 2018-03-05T22:14:32Z

lib/matplotlib/legend.py

+    the :class:`~matplotlib.patches.FancyBboxPatch` which
+    makes up the legend's background.
+    Default is ``None`` which will take the value from the
+    ``legend.fancybox`` :data:`rcParam<matplotlib.rcParams>`.


jklymak · 2018-03-05T22:15:03Z

lib/matplotlib/legend.py

+    Default is ``None`` which will take the value from the
+    ``legend.facecolor`` :data:`rcParam<matplotlib.rcParams>`.
+    If ``"inherit"``, it will take the ``axes.facecolor``
+    :data:`rcParam<matplotlib.rcParams>`.


I'll not note the rest of these. These should all be reverted.

jklymak · 2018-03-05T22:20:14Z

lib/matplotlib/legend_handler.py

+                handler = legend.get_legend_handler(handler_map, handle1)
+
+            _a_list = handler.create_artists(legend, handle1,
+                                             six.next(xds_cycle),


We don't need six anymore. This will be py3 only....

Can you explain (like I'm dumb) why we need widths to cycle here? I'm not grokking the width_ratios and why they are needed.

my fault for the six, i merged with the other pr without seeking the py2->py3 changes

this was pulled from #8292 and i played with it in order to find a reason to keep it.
basically, in the use case were a user would like both an arrow AND text in the legend, it would show up as

and not as this:

and the "sections" that you were asking for further explanation for, are what overlap in the latter picture.
When the widths aren't cycled, the part of the legend where a text or an arrow would show up for the annotation would be drawn on top of one another.
So width_ratios was defined so that each arrow "section" and text "section" would be given their respective space in the legend box in the appropriate location, where their respective widths would be cycled in order to place them both together.

Ok this seems great. but some of this should be in the doc string so it’s clear. Sections is a bit vague.

Cool, if my explanation to you was good enough, ill write it in there :)

Could be shorter. Ie width_ratios specifies the restive widths of a text/arrow legend annotation pair

Gotcha 👍
thanks again!
last thing left on my list is writing better tests :)

jklymak · 2018-03-05T22:22:24Z

lib/matplotlib/legend_handler.py

@@ -649,11 +650,26 @@ class HandlerTuple(HandlerBase):
    pad : float, optional
        If None, fall back to ``legend.borderpad`` as the default.
        In units of fraction of font size. Default is None.
+
+
+    width_ratios : tuple, optional


The term "section" here is opaque to me. What is a "section" and why does it need different widths?

jklymak · 2018-03-05T22:24:14Z

lib/matplotlib/tests/test_legend.py

@@ -250,6 +250,15 @@ def test_hatching():
    ax.legend(handlelength=4, handleheight=4)


+@image_comparison(baseline_images=['legend_arrow_annotation'])


add extensions=['png'], style='mpl20'

jklymak · 2018-03-05T22:25:09Z

lib/matplotlib/tests/test_legend.py

+    # Related to issue 8236
+    # Tests arrow annotations showing up in legend
+    fig, ax = plt.subplots()
+    ax.annotate("foo", (0, 1), xytext=(2, 3), arrowprops={}, label='bar')


please add:

ax.set_xtick_labels('')
ax.set_ytick_labels('')

This helps us cut down on extra text comparison failures.

This test seems a bit minimal. Might as well test all the new artists, as in your example in the PR (or a good subset of them).

tacaswell · 2018-03-06T20:39:26Z

thanks for working on this @magedhennawy !

Changes made. Haven't re-reviewed

jklymak · 2018-03-15T17:24:11Z

You must have a typo somewhere, because this doesn't run at all now!

magedhennawy · 2018-03-15T18:03:38Z

taking a look

jklymak · 2018-03-15T19:11:32Z

you've used mpatches but not defined it.
you changed one of three identical legend parameter list docstrings (yes its annoying, but for now you need to make them all the same).
you have some PEP8 errors.

magedhennawy · 2018-03-15T19:29:45Z

@jklymak im not entirely sure what your second point is referring too :/

jklymak · 2018-03-15T19:32:48Z

lib/matplotlib/legend.py

@@ -428,7 +430,7 @@ def __init__(self, parent, handles, labels,
            The vertical offset (relative to the font size) for the markers
            created for a scatter plot legend entry. 0.0 is at the base the
            legend text, and 1.0 is at the top. To draw all markers at the
-            same height, set to ``[0.5]``. Default is ``[0.375, 0.5, 0.3125]``.
+            same height, set to ``[0.5]``. Default ``[0.375, 0.5, 0.3125]``.


You changed this docstring here. If you want to make the change, you also need to change it in figure.py and axes/_axes.py

ah. okay. gotcha, ill keep it consistent to how the other files are.

Thanks again.

Yes, see #10799 for the proposed fix to this lamentable state of affairs. But in this case, I'd just remove the change.

jklymak · 2018-03-16T04:30:13Z

Now I'm confused - who is authoring this PR?

bursu · 2018-03-16T04:55:14Z

We are working on it together, picking up since he had something to do.

magedhennawy · 2018-03-16T06:39:04Z

@jklymak i had to step out for an event, so my partner stepped in for me :)

magedhennawy · 2018-03-19T17:29:14Z

@jklymak finally. sorry it took a while

jklymak · 2018-03-19T17:31:32Z

lib/matplotlib/tests/test_legend.py

+
+@image_comparison(baseline_images=['legend_all_annotation_text'],
+                  extensions=['png'],
+                  style='mpl20')


Why the other tests rather than just this one?

Not entirely sure, I'm thinking that he wanted to have them all tested separately and then just a cumulative one, also as a "how to use" kind of thing

Image tests have a time cost and a repo-space cost. I'd remove the redundatn ones unless there is a pressing need for them.

got it, push incoming.

jklymak · 2018-03-19T17:51:21Z

Can you guys squash your commits? Please (please) make a local backup branch before you try unless you are very comfortable w/ what you are doing. Please see the instructions in the developers guide.

magedhennawy · 2018-03-19T23:14:47Z

@jklymak the squash is going to be very messy and i'm unsure as to how ill do it properly due to the merges amidst my commits.
Do you have another idea in order for me to do it as efficiently as possible?? :(

jklymak · 2018-03-19T23:20:01Z

Suggest you

make a backup branch
copy your 4 changed files somewhere temporary out of your github repository
assuming you have matplotlib/matplotlib already set up as upstream and have fetched it recently:
- on this branch (issue-8236-dev): git reset --hard upstream/master
copy the changed files back into the proper spots
recommit and then repush to github.

magedhennawy · 2018-03-19T23:36:59Z

@jklymak works perfectly. thank you 👍

jklymak · 2018-03-19T23:39:17Z

No problem. I only had to be told how to do that 10 or 12 times on here 😉

magedhennawy · 2018-03-27T00:08:52Z

does this still need revision?

jklymak

LGTM asside from a couple of formatting issues that are not blockers but could be addressed if there is a more stringent review.

jklymak · 2018-05-02T19:49:52Z

lib/matplotlib/legend_handler.py

-                 handlebox.xdescent, handlebox.ydescent,
-                 handlebox.width, handlebox.height,
-                 fontsize)
+            legend, orig_handle,


Please leave the double indent - this makes it easier to tell its an indent vs a code block...

will be changed asap! :)

jklymak · 2018-05-02T19:50:35Z

lib/matplotlib/legend_handler.py

@@ -441,6 +451,7 @@ class HandlerErrorbar(HandlerLine2D):
    """
    Handler for Errorbars.
    """
+


Were all these extra carriage returns called for somehow?

I believe the pep8 rules that I followed required a blank line after dosctrings in methods, correct me if i'm wrong please.

jklymak · 2018-05-02T19:53:31Z

Don't be shy about pinging for reviews (on a reasonable basis) - we have a somewhat stringent review policy, but not enough reviewers, and github tends to make it too easy for old PRs to get burried.

ImportanceOfBeingErnest · 2018-05-02T20:12:49Z

This is great stuff. I would think this is valuable enough to deserve a What's new entry.

magedhennawy · 2018-05-02T20:15:35Z

Got it! Thank you guys for all your feedback

tacaswell · 2018-07-09T03:55:14Z

Took the liberty of adding a whats new entry, @magedhennawy please feel free to tweak it!

tacaswell · 2018-07-09T03:57:15Z

lib/matplotlib/legend_handler.py

+        self._handlers = handlers
+
+        if (width_ratios is not None) and (len(width_ratios) == ndivide):
+            self._width_ratios = width_ratios


Can you raise here if the width_rations and ndivide are not compatible? I can imaging this being a very frustrating bug to track down where we are ignoring one of the inputs!

tacaswell · 2018-07-09T04:00:04Z

lib/matplotlib/legend_handler.py


        self._ndivide = ndivide
        self._pad = pad
+        self._handlers = handlers


Can you validate that handlers is the correct length? That way we can raise an exception as early as possible.

tacaswell

Small changes to the __init__ to validate the inputs that have to be the same length.

jklymak · 2018-08-17T00:40:00Z

ping @magedhennawy !

jklymak · 2018-10-06T05:19:35Z

pinging again...

jklymak · 2019-07-16T19:30:30Z

Thanks a lot for the contribution. I'm going to close this as having had no action for quite a while. OTOH, if there is still a community that wants this, please feel free to re-open, with my apologies!

jklymak added this to the v3.0 milestone Mar 5, 2018

jklymak previously requested changes Mar 5, 2018

View reviewed changes

jklymak added the status: needs revision label Mar 7, 2018

jklymak reviewed Mar 15, 2018

View reviewed changes

jklymak mentioned this pull request Mar 15, 2018

Legend annotation #10802

Closed

6 tasks

jklymak reviewed Mar 19, 2018

View reviewed changes

Add legend handler and artist for FancyArrow (clean commit)

4c2e1d6

magedhennawy force-pushed the issue-8236-dev branch from 8b5b650 to 4c2e1d6 Compare March 19, 2018 23:36

jklymak added status: needs review and removed status: needs revision labels Mar 27, 2018

jklymak approved these changes May 2, 2018

View reviewed changes

jklymak mentioned this pull request May 17, 2018

Annotation legend #8292

Closed

DOC: add whats_new entry

6cf5e2b

tacaswell reviewed Jul 9, 2018

View reviewed changes

tacaswell modified the milestones: v3.0, v3.1 Jul 9, 2018

tacaswell requested changes Jul 9, 2018

View reviewed changes

tacaswell modified the milestones: v3.0, v3.1 Jul 15, 2018

jklymak removed the status: needs review label Aug 17, 2018

Wojciech-Lange mentioned this pull request Dec 21, 2018

Legend shows arrow from annotate #8236 #13034

Closed

3 tasks

jklymak added the stale label Feb 10, 2019

jklymak modified the milestones: v3.1.0, v3.2.0 Feb 10, 2019

jklymak closed this Jul 16, 2019

oscargus mentioned this pull request May 29, 2022

Add legend handler for Annotation #23160

Draft

6 tasks

		@@ -250,6 +250,15 @@ def test_hatching():
		ax.legend(handlelength=4, handleheight=4)


		@image_comparison(baseline_images=['legend_arrow_annotation'])

Add legend handler and artist for FancyArrow #10688

Add legend handler and artist for FancyArrow #10688

Conversation

magedhennawy commented Mar 5, 2018 • edited Loading

jklymak commented Mar 5, 2018

magedhennawy commented Mar 5, 2018

jlecoeur commented Mar 5, 2018

magedhennawy commented Mar 5, 2018

jklymak left a comment • edited Loading

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

magedhennawy Mar 6, 2018 • edited Loading

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

Choose a reason for hiding this comment

tacaswell commented Mar 6, 2018

jklymak commented Mar 15, 2018

magedhennawy commented Mar 15, 2018

jklymak commented Mar 15, 2018

magedhennawy commented Mar 15, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jklymak commented Mar 16, 2018

bursu commented Mar 16, 2018

magedhennawy commented Mar 16, 2018

magedhennawy commented Mar 19, 2018

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 Mar 19, 2018

magedhennawy commented Mar 19, 2018

jklymak commented Mar 19, 2018

magedhennawy commented Mar 19, 2018

jklymak commented Mar 19, 2018

magedhennawy commented Mar 27, 2018

jklymak left a 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 May 2, 2018

ImportanceOfBeingErnest commented May 2, 2018

magedhennawy commented May 2, 2018

tacaswell commented Jul 9, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tacaswell left a comment

Choose a reason for hiding this comment

jklymak commented Aug 17, 2018

jklymak commented Oct 6, 2018

jklymak commented Jul 16, 2019

magedhennawy commented Mar 5, 2018 •

edited

Loading

jklymak left a comment •

edited

Loading

magedhennawy Mar 6, 2018 •

edited

Loading