Issues warnings for legend handles without handlers

[kdpenner]

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:

1. Text with a label is found
2. 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]

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]

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

[tacaswell]

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

[kdpenner]

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.

[tacaswell]

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]

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.

[kdpenner]

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.

[tacaswell]

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.

[kdpenner]

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.

[tacaswell]

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]

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

[tacaswell]

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]

ping @kdpenner, this is draft again ;-)

[kdpenner]

@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]

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

[kdpenner]

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:

1. 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):

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

[kdpenner]

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]

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

[kdpenner]

@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]

At the least it must pass the test suite.

[kdpenner]

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

[kdpenner]

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]

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

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

[tacaswell]

Hopefully we will hear from you agian

[kdpenner]

Thanks!