Issues warnings for legend handles without handlers #20470
Conversation
|
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. |
|
Ooops apologies. I thought folks get those notifications. In the future I will be sure to also comment. |
lib/matplotlib/legend.py
Outdated
| if isinstance(a, (Line2D, Patch, Collection, Text))), | ||
| *axx.containers] | ||
|
|
||
| handler_map = Legend.get_default_handler_map() |
There was a problem hiding this comment.
Why did you remove this whole code block? Without this here one of the inputs in now completely and silently ignored.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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 ?
lib/matplotlib/legend.py
Outdated
| "tutorials/intermediate/legend_guide.html" | ||
| "#implementing-a-custom-legend-handler") | ||
| continue | ||
| else: |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
This is called from
matplotlib/lib/matplotlib/legend.py
Lines 493 to 494 in 3e32cc7
matplotlib/lib/matplotlib/legend.py
Lines 401 to 411 in 3e32cc7
matplotlib/lib/matplotlib/legend.py
Lines 294 to 295 in 3e32cc7
matplotlib/lib/matplotlib/axes/_axes.py
Lines 299 to 305 in 3e32cc7
matplotlib/lib/matplotlib/legend.py
Lines 1210 to 1211 in 3e32cc7
matplotlib/lib/matplotlib/legend.py
Lines 1240 to 1242 in 3e32cc7
handles here so we can not assume that t
8000
he input types will be limited to what the auto-discovery code picks up.
There was a problem hiding this comment.
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
and it wouldn't need to fall back to a Rectangle.
There was a problem hiding this comment.
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.
|
I have to update the PR to accommodate the new |
lib/matplotlib/legend.py
Outdated
| for handle in handles_original: | ||
| label = handle.get_label() | ||
| if label != '_nolegend_' and has_handler(handler_map, handle): | ||
| if label not in ['_nolegend_', '']: |
There was a problem hiding this comment.
We currently do the filtering to "non empty labels" in
matplotlib/lib/matplotlib/legend.py
Lines 1139 to 1152 in 3e32cc7
There was a problem hiding this comment.
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.
|
ping @kdpenner, this is draft again ;-) |
7d43704 to
e82af34
Compare
|
@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() |
|
This has a failing test: lib/matplotlib/tests/test_axes.py::test_pie_nolabel_but_legend[png] |
|
That test fails because it doesn't expect the warning.
So there are three options:
matplotlib/lib/matplotlib/legend.py Line 1124 in e82af34
|
|
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. |
|
Popping to draft until you sort the above out. I think @timhoffm was the proponent of this warning. |
|
@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. |
|
At the least it must pass the test suite. |
|
Docs fail but I'm not sure why. They render fine. 4 warnings are raised. All other tests pass. |
c3cd473 to
01a85b9
Compare
|
do I need to mark the PR as ready for review for the workflows to run? |
Major changes to PR render review obsolete
There was a problem hiding this comment.
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...
|
Thank you @kdpenner and congratulations on your first merged matplotlib PR 🎉 Thank you for persevering with us through what was a long process! |
|
Hopefully we will hear from you agian |
|
Thanks! |
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:
PR Checklist
pytestpasses).flake8on changed files to check).flake8-docstringsand runflake8 --docstring-convention=all).doc/users/next_whats_new/(follow instructions in README.rst there).doc/api/next_api_changes/(follow instructions in README.rst there).