Partly revert #27711 #27780

timhoffm · 2024-02-12T23:07:56Z

This PR removes the propagation of boxplot(..., labels=...) to any artist legend labels introduced in #27711.

Other than the rest of the plotting functions labels is not used for legend labels but for xtick labels. This is only poorly documented via https://matplotlib.org/stable/api/_as_gen/matplotlib.axes.Axes.bxp.html and in an example.

Whatever our way forward regarding the use of labels is, we should by no means propagate them simultaneously to xticks and legend entries. This coupling would cripple users' configurability and limit our ability to migrate to a clear API where legend labels and tick labels can be configured independently.

Until we have sorted out a better API (xref #27767 (comment)), the recommended solution for the original issue #20512 is to grab the artists returned from boxplot() and either set_label() on them or pass them to the legend call ax.legend(handles, labels).

In particular ping @tacaswell @dstansby who have approved #27711.

tacaswell · 2024-02-12T23:33:21Z

I thought it was ok to propagate because there is a keyword to control if boxplot manages the ticks or not.

timhoffm · 2024-02-13T00:10:57Z

Uh, ok, I did not know about that ... 🤯

I suppose, manage_ticks isn't a justification though. First, it's quite an awkward API to change the effect of one parameter (labels) through another (manage_ticks). Second, you still cannot put labels to ticks, but not get legend entries through the boxplot API.

But let me rethink what the implications for usability and future API changes are. I'll put to draft.

tacaswell · 2024-02-13T15:42:28Z

To elaborate more, my thinking was that users would either use do nothing (and we manage the ticks) or pass manage_ticks=False and use ax.legend(). However, I did not thoroughly think through the consequences.

If we are messing with the ticks, putting any other artists on the axes is likely to be a brittle so I am not sure how common it would be to have a combination of a boxplot which manages the ticks and some other artists on the axes which requires a legend.

timhoffm · 2024-02-13T22:54:32Z

TL;DR: Irrespective of the tick vs. legend labelling issue, the current implementation (#27711) is not generally suitable for legend labels.

After testing:
The current implementation was motivated by and tested with a very special usage scenario, where you make one box per boxplot() call only (see the test removed in this PR).

In more general scenarios (tested here by adding legend() to the recently rewritten https://matplotlib.org/devdocs/gallery/statistics/boxplot_color.html), one gets

and without re-coloring the individual bars:

In particular:

The labels parameter is verbatimly copied to each box. - Whether or not one re-styles the boxes, it's not reasonable to have the same label on multiple legend entries.
boxplot does not allow individual colors per box in its call. So, by default we have all N identical styled boxes in the legend. Even if we would fix (1) and distribute the labels elements to these boxes, the boxes would look the same with different labels, which is also not helpful.

So #27711 is only helpful for one very particular use case, but does not yield reasonable legends in other more standard cases. I therefore, suggest to revert it (-> undrafted this PR) and separately start thinking on more scalable approaches.

tacaswell · 2024-02-14T16:48:54Z

oh, I agree that is broken!

I thought that vector-like inputs were broadcast across boxplots....

rcomer · 2024-02-14T16:51:37Z

FWIW I think my second commit at #27767 would fix the first fruity case above.

dstansby

Thanks for this PR and the clear explanation/discussion. Agreed that the current (main) behaviour is not what we want, and that it's worth reverting this.

Long term I think the right answer is to have one legend label per boxplot call, perhaps from a new legend_label: str argument? Beacuse the format is the same for all the drawn boxplots when boxplot is called, this should be fine, and if users manually change the formatting of the boxplots (as in https://matplotlib.org/devdocs/gallery/statistics/boxplot_color.html) that's their lookout (in the same way if you manually change the colour of one patch that scatter generates you shouldn't expect Matplotlib to reflect that change in the legend entry). But we can defer this disucssion to another issue 😄

Marking as request changes because I think you need an ax.legend() call in the test to actually test the new behaviour.

lib/matplotlib/axes/_axes.py

dstansby · 2024-02-14T16:44:23Z

lib/matplotlib/tests/test_legend.py

+    # Check that labels set the tick labels ...
+    assert [l.get_text() for l in ax.get_xticklabels()] == ['A', 'B', 'C']
+    # ... but not legend entries
+    handles, labels = ax.get_legend_handles_labels()


Doesn't this need an ax.legend() call first to properley test if any legend entries are added? It seems to work as intended, but locally I get a UserWarning: No artists with labels found to put in legend. Note that artists whose label start with an underscore are ignored when legend() is called with no argument. which is expected and I think should be caught in this test when ax.legend() is called.

get_legend_handles_and_labels() is the "give me all entries for a legend" function. It traverses all artists and return handle+label for those that should be included in a legend because they have a label. Compare

ax.legend() calls that internally. IMHO it's more straight forward to just check that there are no artists for a legend that to try and create a legend and check that this warns that there are no entries.

dstansby

See above - forgot to select "request changes"

This PR removes the propagation of `labels` to any artist legend labels. Other than the rest of the plotting functions `labels` is not used for legend labels but for xtick labels. This is only poorly documented via https://matplotlib.org/stable/api/_as_gen/matplotlib.axes.Axes.bxp.html and in an [example](https://matplotlib.org/stable/gallery/statistics/boxplot_color.html). Whatever our way forward regarding the use of `labels` is, we should by no means propagate them simultaneously to xticks and legend entries. This coupling would cripple users' configurability and limit our ability to migrate to a clear API where legend labels and tick labels can be configured independently. Until we have sorted out a better API, the recommended solution for the original issue matplotlib#20512 is to grab the artists returned from `boxplot()` and either `set_label()` on them or pass them to the legend call `ax.legend(handles, labels)`.

timhoffm · 2024-02-14T19:44:50Z

FWIW I think my second commit at #27767 would fix the first fruity case above.

Yes, but unfortunately it does not generalize well to the more common second case. And there's still helf of the tick/legend duplicity issue.

Long term I think the right answer is to have one legend label per boxplot call

Fundamentally yes! Naming is an issue though, but let's discuss solutions in a separate issue. ~~I'll open one.~~ Let's discuss in #27792.

story645 · 2024-02-14T20:12:27Z

boxplot does not allow individual colors per box in its call.

#27304 needs a decision and I think is worth pulling back to boxplot too - meaning if the consensus is color keywords do that in boxplot, if consensus is prop keywords do that.

timhoffm · 2024-02-14T21:37:40Z

boxplot does not allow individual colors per box in its call.

#27304 needs a decision and I think is worth pulling back to boxplot too

That's orthogonal to this PR, which just cleans up an improper API extension to our current functionality. For new labelling API, please discuss #27792. In particular note, that we could handle different-colored boxes in an extension as well (#27792 (comment)). So, you'd be covered.

dstansby · 2024-02-14T22:45:21Z

Since the only change since @tacaswell's approval was a very minor docstring wording update, I'll merge this with two approvals.

timhoffm added the Release critical For bugs that make the library unusable (segfaults, incorrect plots, etc) and major regressions. label Feb 12, 2024

timhoffm added this to the v3.9.0 milestone Feb 12, 2024

timhoffm mentioned this pull request Feb 12, 2024

Update handling of sequence labels for plot #27767

Merged

4 tasks

timhoffm force-pushed the fix-boxplot-labels branch from 12527a7 to 8581c19 Compare February 12, 2024 23:18

timhoffm marked this pull request as draft February 13, 2024 00:11

timhoffm marked this pull request as ready for review February 13, 2024 22:54

tacaswell approved these changes Feb 14, 2024

View reviewed changes

dstansby reviewed Feb 14, 2024

View reviewed changes

dstansby requested changes Feb 14, 2024

View reviewed changes

timhoffm force-pushed the fix-boxplot-labels branch from 8581c19 to 3dfcb51 Compare February 14, 2024 19:34

timhoffm mentioned this pull request Feb 14, 2024

[ENH]: Legend entries for boxplot #27792

Closed

dstansby approved these changes Feb 14, 2024

View reviewed changes

dstansby merged commit 4052a10 into matplotlib:main Feb 14, 2024

timhoffm deleted the fix-boxplot-labels branch February 14, 2024 22:52

timhoffm mentioned this pull request Feb 14, 2024

Fix boxplot legend entries part 2 #27711

Merged

5 tasks

saranti mentioned this pull request Mar 1, 2024

Add legend support for boxplots #27840

Merged

5 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Partly revert #27711 #27780

Partly revert #27711 #27780

timhoffm commented Feb 12, 2024 •

edited

Loading

tacaswell commented Feb 12, 2024

timhoffm commented Feb 13, 2024 •

edited

Loading

tacaswell commented Feb 13, 2024

timhoffm commented Feb 13, 2024 •

edited

Loading

tacaswell commented Feb 14, 2024

rcomer commented Feb 14, 2024 •

edited

Loading

dstansby left a comment

dstansby Feb 14, 2024

timhoffm Feb 14, 2024 •

edited

Loading

dstansby left a comment

timhoffm commented Feb 14, 2024 •

edited

Loading

story645 commented Feb 14, 2024

timhoffm commented Feb 14, 2024 •

edited

Loading

dstansby commented Feb 14, 2024

Partly revert #27711 #27780

Partly revert #27711 #27780

Conversation

timhoffm commented Feb 12, 2024 • edited Loading

tacaswell commented Feb 12, 2024

timhoffm commented Feb 13, 2024 • edited Loading

tacaswell commented Feb 13, 2024

timhoffm commented Feb 13, 2024 • edited Loading

tacaswell commented Feb 14, 2024

rcomer commented Feb 14, 2024 • edited Loading

dstansby left a comment

Choose a reason for hiding this comment

dstansby Feb 14, 2024

Choose a reason for hiding this comment

timhoffm Feb 14, 2024 • edited Loading

Choose a reason for hiding this comment

dstansby left a comment

Choose a reason for hiding this comment

timhoffm commented Feb 14, 2024 • edited Loading

story645 commented Feb 14, 2024

timhoffm commented Feb 14, 2024 • edited Loading

dstansby commented Feb 14, 2024

timhoffm commented Feb 12, 2024 •

edited

Loading

timhoffm commented Feb 13, 2024 •

edited

Loading

timhoffm commented Feb 13, 2024 •

edited

Loading

rcomer commented Feb 14, 2024 •

edited

Loading

timhoffm Feb 14, 2024 •

edited

Loading

timhoffm commented Feb 14, 2024 •

edited

Loading

timhoffm commented Feb 14, 2024 •

edited

Loading