Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Partly revert #27711 #27780

Merged
merged 1 commit into from
Feb 14, 2024
Merged

Partly revert #27711 #27780

merged 1 commit into from
Feb 14, 2024

Conversation

timhoffm
Copy link
Member

@timhoffm timhoffm commented Feb 12, 2024

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.

@timhoffm timhoffm added the Release critical For bugs that make the library unusable (segfaults, incorrect plots, etc) and major regressions. label Feb 12, 2024
@timhoffm timhoffm added this to the v3.9.0 milestone Feb 12, 2024
@tacaswell
Copy link
Member

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

@timhoffm
Copy link
Member Author

timhoffm commented Feb 13, 2024

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.

@timhoffm timhoffm marked this pull request as draft February 13, 2024 00:11
@tacaswell
Copy link
Member

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
Copy link
Member Author

timhoffm commented Feb 13, 2024

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

image

and without re-coloring the individual bars:

image

In particular:

  1. 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.
  2. 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.

@timhoffm timhoffm marked this pull request as ready for review February 13, 2024 22:54
@tacaswell
Copy link
Member

oh, I agree that is broken!

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

@rcomer
Copy link
Member

rcomer commented Feb 14, 2024

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

Copy link
Member

@dstansby dstansby left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 Outdated Show resolved Hide resolved
# 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()
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member Author

@timhoffm timhoffm Feb 14, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

grafik

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.

Copy link
Member

@dstansby dstansby left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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
Copy link
Member Author

timhoffm commented Feb 14, 2024

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
Copy link
Member

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
Copy link
Member Author

timhoffm commented Feb 14, 2024

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
Copy link
Member

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

@dstansby dstansby merged commit 4052a10 into matplotlib:main Feb 14, 2024
42 checks passed
@timhoffm timhoffm deleted the fix-boxplot-labels branch February 14, 2024 22:52
@timhoffm timhoffm mentioned this pull request Feb 14, 2024
5 tasks
@saranti saranti mentioned this pull request Mar 1, 2024
5 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Release critical For bugs that make the library unusable (segfaults, incorrect plots, etc) and major regressions.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants