Issues warnings for legend handles without handlers #20470

kdpenner · 2021-06-19T22:21:18Z

PR Summary

Closes #19121.

Text was not included in the Artists collected for a legend, so text created with a label had no legend entry. This PR issues warnings when:

Text with a label is found
a rectangular handle is substituted for a Patch that is not a Rectangle

PR Checklist

Has pytest style unit tests (and pytest passes).
Is Flake 8 compliant (run flake8 on changed files to check).
[N/A] New features are documented, with examples if plot related.
[N/A] Documentation is sphinx and numpydoc compliant (the docs should build without error).
Conforms to Matplotlib style conventions (install flake8-docstrings and run flake8 --docstring-convention=all).
[N/A] New features have an entry in doc/users/next_whats_new/ (follow instructions in README.rst there).
[N/A] API changes documented in doc/api/next_api_changes/ (follow instructions in README.rst there).

kdpenner · 2021-08-06T16:17:47Z

Ready for review. FYI I received no notification that the PR was marked as a draft and assigned a status of needs tests until I checked up on my PRs.

jklymak · 2021-08-06T16:25:17Z

Ooops apologies. I thought folks get those notifications. In the future I will be sure to also comment.

tacaswell · 2021-08-06T16:54:42Z

lib/matplotlib/legend.py

+                      if isinstance(a, (Line2D, Patch, Collection, Text))),
                    *axx.containers]

-    handler_map = Legend.get_default_handler_map()


Why did you remove this whole code block? Without this here one of the inputs in now completely and silently ignored.

Hm. I don't remember the call sequence well enough to know why I did it in the first place. But I have an example where I pass a handler_map dict in the legend call and this block doesn't update the handler map:

ax.legend(handles, labels, handler_map={Text: HandlerText()})

so I might've deleted it because it did nothing.

That may be evidence that there is a bug someplace else that is making it so the additional handler map is not getting routed.

I suspect that the change we want to make in this function is to drop the filtering on L1144 all together, put this block of code back, filter out the things with _nolegend_ as the label, warn on things that do not have a handler (but drop them on the floor), and then restore the behavior of only yielding things that have handlers (that way we can give a different warning for auto-discovered things rather than explicitly passed things).

If we do this then I do not think we need any changes to _init_legend_box ?

tacaswell · 2021-08-06T17:00:19Z

lib/matplotlib/legend.py

+                            "tutorials/intermediate/legend_guide.html"
+                            "#implementing-a-custom-legend-handler")
+                    continue
+                else:


This is going to provide switch every Artists that is not Text to fallback to Rectangle! That seems a bit over-broad and I am not sure the Rectangle handler will be able to extract the information it needs from an arbitrary Artist.

The only Artists that are iterated over by default are: Line2D, Patch, Collection, and Text.

Line2D and (some?) Collection are in the _default_handler_map.

Text cannot be put in _default_handler_map unless a handler is written for it, and the solution for the linked issue was scoped to issuing a warning.

Before this PR, any Patch mapped to a HandlerPatch, which was a Rectangle. I've made that behavior explicit.

This is called from

matplotlib/lib/matplotlib/legend.py

Lines 493 to 494 in 3e32cc7

# init with null renderer

self._init_legend_box(handles, labels, markerfirst)

which are lightly processed at

matplotlib/lib/matplotlib/legend.py

Lines 401 to 411 in 3e32cc7

for label, handle in zip(labels, handles):

if isinstance(label, str) and label.startswith('_'):

_api.warn_external('The handle {!r} has a label of {!r} '

'which cannot be automatically added to'

' the legend.'.format(handle, label))

else:

_lab.append(label)

_hand.append(handle)

labels, handles = _lab, _hand

handles = list(handles)

but otherwise come in from

matplotlib/lib/matplotlib/legend.py

Lines 294 to 295 in 3e32cc7

def __init__(

self, parent, handles, labels,

which is in turn called from

matplotlib/lib/matplotlib/axes/_axes.py

Lines 299 to 305 in 3e32cc7

handles, labels, extra_args, kwargs = mlegend._parse_legend_args(

[self],

*args,

**kwargs)

if len(extra_args):

raise TypeError('legend only accepts two non-keyword arguments')

self.legend_ = mlegend.Legend(self, handles, labels, **kwargs)

which has the path

matplotlib/lib/matplotlib/legend.py

Lines 1210 to 1211 in 3e32cc7

if handles and labels:

handles, labels = zip(*zip(handles, labels))

and

matplotlib/lib/matplotlib/legend.py

Lines 1240 to 1242 in 3e32cc7

elif len(args) >= 2:

handles, labels = args[:2]

extra_args = args[2:]

that means the user can (eventually) get arbitrary artists into handles here so we can not assume that the input types will be limited to what the auto-discovery code picks up.

But the arbitrary artist needs a handler in the handler map for it to be rendered in the legend; can a user pass an arbitrary artist in this way and expect the handler to be rendered without an entry in the map? So handler would not be None here:

matplotlib/lib/matplotlib/legend.py

Line 765 in 7d43704

handler = self.get_legend_handler(legend_handler_map, orig_handle)

and it wouldn't need to fall back to a Rectangle.

However if the user _does_make a mistake and pass in an artist that does not have a handler entry we should not then fallback to Rectangle, we should maintain the current behavior of warning and passing on the handle.

kdpenner · 2021-08-06T18:35:00Z

I have to update the PR to accommodate the new Wedge artist.

tacaswell · 2021-08-06T20:05:35Z

lib/matplotlib/legend.py

    for handle in handles_original:
        label = handle.get_label()
-        if label != '_nolegend_' and has_handler(handler_map, handle):
+        if label not in ['_nolegend_', '']:


We currently do the filtering to "non empty labels" in

matplotlib/lib/matplotlib/legend.py

Lines 1139 to 1152 in 3e32cc7

def _get_legend_handles_labels(axs, legend_handler_map=None):

"""

Return handles and labels for legend, internal method.

"""

handles = []

labels = []

for handle in _get_legend_handles(axs, legend_handler_map):

label = handle.get_label()

if label and not label.startswith('_'):

handles.append(handle)

labels.append(label)

return handles, labels

it is probably better if we leave that filtering step there and continue to only do "artist noped out of being in the legend" here.

tacaswell

See my in-line comments. This PR is currently making more extensive changes to behavior than is required to add a warning when a label is added to a Text object.

jklymak · 2021-09-14T11:47:28Z

ping @kdpenner, this is draft again ;-)

kdpenner · 2021-09-14T16:09:10Z

@jklymak thanks for the ping, I got busy with work and let this languish.

@tacaswell this is ready for re-review.

Below are a bunch of usage patterns. ax2 and ax6 are important, as they are workarounds from before this PR, which changes their behavior. ax2 results in a nuisance text warning; previously no warning was issued. ax6 results in an additional entry in the legend.

import matplotlib.pyplot as plt
from matplotlib.text import Text

class HandlerText:
    def legend_artist(self, legend, orig_handle, fontsize, handlebox):
        x0, y0 = handlebox.xdescent, handlebox.ydescent
        handle_text = Text(x=x0, y=y0, text=orig_handle.get_text())
        handlebox.add_artist(handle_text)
        return handle_text

fig = plt.figure()


print("ax1")
ax1 = fig.add_subplot(3, 3, 1)
# From original issue: now text warning is issued
ax1.text(x=2, y=5, s="text", label="label")
ax1.legend()


print("ax2")
ax2 = fig.add_subplot(3, 3, 2)
# With solution from before PR, nuisance text warning is issued
annotation = ax2.text(x=0, y=0, s="text", label="label")
handles, labels = ax2.get_legend_handles_labels()
handles.append(annotation)
labels.append(annotation.get_label())
ax2.legend(handles, labels, handler_map={Text: HandlerText()})


print("ax3")
ax3 = fig.add_subplot(3, 3, 3)
# Text warning is not issued
ax3.text(x=0, y=0, s="text", label="label")
ax3.legend(handler_map={Text: HandlerText()})


print("ax4")
ax4 = fig.add_subplot(3, 3, 4)
# Artists are found but their handlers are not added to the legend because
# they're not in the handler map passed in ax.legend()
ax4.text(x=0, y=0, s="text", label="label")
handles, labels = ax4.get_legend_handles_labels(
                                    legend_handler_map={Text: HandlerText()})
ax4.legend(handles, labels)


print("ax5")
ax5 = fig.add_subplot(3, 3, 5)
# Artists are found and added
ax5.text(x=0, y=0, s="text", label="label")
handles, labels = ax5.get_legend_handles_labels(
                                    legend_handler_map={Text: HandlerText()})
ax5.legend(handles, labels, handler_map={Text: HandlerText()})


print("ax6")
ax6 = fig.add_subplot(3, 3, 6)
from matplotlib.legend import Legend
Legend.update_default_handler_map({Text: HandlerText()})
# two legend entries
annotation = ax6.text(x=0, y=0, s="text", label="label")
handles, labels = ax6.get_legend_handles_labels()
handles.append(annotation)
labels.append(annotation.get_label())
ax6.legend(handles, labels)


print("ax7")
ax7 = fig.add_subplot(3, 3, 7)
# after handler map update, just works
ax7.text(x=0, y=0, s="text", label="label")
ax7.legend()

plt.show()

jklymak · 2021-09-15T12:59:40Z

This has a failing test: lib/matplotlib/tests/test_axes.py::test_pie_nolabel_but_legend[png]

kdpenner · 2021-09-15T15:40:18Z

That test fails because it doesn't expect the warning.

plt.pie creates Text artists with empty strings for the labels. _get_legend_handles_labels is called, which calls _get_legend_handles, which raises the warning. I could filter out the Text artists with label not in ['_nolegend_', ''] in _get_legend_handles, but @tacaswell recommended leaving the filtering step to _get_legend_handles_labels, where artists with labels starting with '_' are dropped.

So there are three options:

use label not in ['_nolegend_', ''] and not has_handler in _get_legend_handles

matplotlib/lib/matplotlib/legend.py

Line 1124 in e82af34

elif not has_handler(handler_map, handle):

create Text artists with _nolegend_ labels and use label != "_nolegend_" and 5B9E not has_handler in _get_legend_handles
modify the test to capture the warning

kdpenner · 2021-09-15T16:49:08Z

Now that I've thought about this more option 1 is the only one for which warnings won't be raised for Text created without an intention of appearing in the legend.

jklymak · 2021-10-04T07:35:53Z

Popping to draft until you sort the above out. I think @timhoffm was the proponent of this warning.

kdpenner · 2021-10-04T11:12:23Z

@jklymak what do I need to sort out? Thought I’d leave those options and discussion for the reviewer.

Also I think the needs test tag can be removed, as there is now a test.

jklymak · 2021-10-04T11:39:53Z

At the least it must pass the test suite.

kdpenner · 2021-10-04T15:00:00Z

Docs fail but I'm not sure why. They render fine. 4 warnings are raised. All other tests pass.

kdpenner · 2021-11-08T16:43:13Z

do I need to mark the PR as ready for review for the workflows to run?

Major changes to PR render review obsolete

jklymak

This seems fine to me - I'm not 100% in favour of warnings like this; surely most folks understand that a text object doesn't need a legend entry. But it may be helpful for other artists...

tacaswell · 2022-03-03T04:38:18Z

Thank you @kdpenner and congratulations on your first merged matplotlib PR 🎉

Thank you for persevering with us through what was a long process!

tacaswell · 2022-03-03T04:38:29Z

Hopefully we will hear from you agian

kdpenner · 2022-03-03T13:22:21Z

Thanks!

jklymak added status: needs tests status: needs revision labels Jul 7, 2021

jklymak marked this pull request as draft July 7, 2021 20:31

kdpenner marked this pull request as ready for review August 6, 2021 16:16

tacaswell reviewed Aug 6, 2021

View reviewed changes

tacaswell previously requested changes Aug 6, 2021

View reviewed changes

jklymak added the status: needs rebase label Sep 14, 2021

jklymak marked this pull request as draft September 14, 2021 11:47

kdpenner force-pushed the legend-handlers branch from 7d43704 to e82af34 Compare September 14, 2021 15:54

kdpenner marked this pull request as ready for review September 14, 2021 16:10

jklymak removed the status: needs rebase label Sep 15, 2021

jklymak marked this pull request as draft October 4, 2021 07:33

tacaswell force-pushed the main branch from f6e0ee4 to a1eef38 Compare October 20, 2021 19:55

Issues warnings for legend handles without handlers

01a85b9

kdpenner force-pushed the legend-handlers branch from c3cd473 to 01a85b9 Compare November 8, 2021 16:15

kdpenner marked this pull request as ready for review November 14, 2021 16:32

kdpenner requested a review from tacaswell November 14, 2021 16:33

tacaswell added this to the v3.6.0 milestone Dec 24, 2021

jklymak approved these changes Feb 1, 2022

View reviewed changes

jklymak added status: needs review and removed status: needs revision status: needs tests labels Feb 1, 2022

tacaswell merged commit a734ed2 into matplotlib:main Mar 3, 2022

QuLogic removed the status: needs review label Mar 3, 2022

kdpenner deleted the legend-handlers branch March 3, 2022 13:22

jklymak mentioned this pull request Aug 27, 2022

[Bug]: False positive legend handler warnings in 3.6.0.rc1 #23761

Closed

	# init with null renderer
	self._init_legend_box(handles, labels, markerfirst)

	for label, handle in zip(labels, handles):
	if isinstance(label, str) and label.startswith('_'):
	_api.warn_external('The handle {!r} has a label of {!r} '
	'which cannot be automatically added to'
	' the legend.'.format(handle, label))
	else:
	_lab.append(label)
	_hand.append(handle)
	labels, handles = _lab, _hand

	handles = list(handles)

	handles, labels, extra_args, kwargs = mlegend._parse_legend_args(
	[self],
	*args,
	**kwargs)
	if len(extra_args):
	raise TypeError('legend only accepts two non-keyword arguments')
	self.legend_ = mlegend.Legend(self, handles, labels, **kwargs)

	if handles and labels:
	handles, labels = zip(*zip(handles, labels))

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

	def _get_legend_handles_labels(axs, legend_handler_map=None):
	"""
	Return handles and labels for legend, internal method.

	"""
	handles = []
	labels = []

	for handle in _get_legend_handles(axs, legend_handler_map):
	label = handle.get_label()
	if label and not label.startswith('_'):
	handles.append(handle)
	labels.append(label)
	return handles, labels

Uh oh!

Issues warnings for legend handles without handlers #20470

Issues warnings for legend handles without handlers #20470

Uh oh!

Conversation

Uh oh!

PR Summary

PR Checklist

Uh oh!

Uh oh!

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!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants