Movatterモバイル変換


[0]ホーム

URL:


Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

Sign up
Appearance settings

Partly revert #27711#27780

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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.

Already on GitHub?Sign in to your account

Merged
dstansby merged 1 commit intomatplotlib:mainfromtimhoffm:fix-boxplot-labels
Feb 14, 2024
Merged

Conversation

timhoffm
Copy link
Member

@timhoffmtimhoffm commentedFeb 12, 2024
edited
Loading

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

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

Whatever our way forward regarding the use oflabels 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 fromboxplot() and eitherset_label() on them or pass them to the legend callax.legend(handles, labels).

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

@timhoffmtimhoffm added the Release criticalFor bugs that make the library unusable (segfaults, incorrect plots, etc) and major regressions. labelFeb 12, 2024
@timhoffmtimhoffm added this to thev3.9.0 milestoneFeb 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
MemberAuthor

timhoffm commentedFeb 13, 2024
edited
Loading

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 theboxplot API.

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

@timhoffmtimhoffm marked this pull request as draftFebruary 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 passmanage_ticks=False and useax.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
MemberAuthor

timhoffm commentedFeb 13, 2024
edited
Loading

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 perboxplot() call only (see the test removed in this PR).

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

image

and without re-coloring the individual bars:

image

In particular:

  1. Thelabels 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 thelabels 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.

@timhoffmtimhoffm marked this pull request as ready for reviewFebruary 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 commentedFeb 14, 2024
edited
Loading

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

Copy link
Member

@dstansbydstansby 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 newlegend_label: str argument? Beacuse the format is the same for all the drawn boxplots whenboxplot is called, this should be fine, and if users manually change the formatting of the boxplots (as inhttps://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 thatscatter 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 anax.legend() call in the test to actually test the new behaviour.

# 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 anax.legend() call first to properley test if any legend entries are added? It seems to work as intended, but locally I get aUserWarning: 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 whenax.legend() is called.

Copy link
MemberAuthor

@timhoffmtimhoffmFeb 14, 2024
edited
Loading

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

@dstansbydstansby 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 labelsbut for xtick labels. This is only poorly documented viahttps://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 meanspropagate them simultaneously to xticks and legend entries. This coupling would crippleusers' configurability and limit our ability to migrate to a clear API where legendlabels and tick labels can be configured independently.Until we have sorted out a better API, the recommended solution for the original issuematplotlib#20512 is to grab the artists returned from `boxplot()` and either `set_label()` onthem or pass them to the legend call `ax.legend(handles, labels)`.
@timhoffm
Copy link
MemberAuthor

timhoffm commentedFeb 14, 2024
edited
Loading

FWIW I think mysecond 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.

rcomer reacted with thumbs up emoji

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

timhoffm commentedFeb 14, 2024
edited
Loading

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.

story645 reacted with thumbs up emoji

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

tacaswell reacted with thumbs up emoji

@dstansbydstansby merged commit4052a10 intomatplotlib:mainFeb 14, 2024
@timhoffmtimhoffm deleted the fix-boxplot-labels branchFebruary 14, 2024 22:52
@timhoffmtimhoffm mentioned this pull requestFeb 14, 2024
5 tasks
@sarantisaranti mentioned this pull requestMar 1, 2024
5 tasks
Sign up for freeto join this conversation on GitHub. Already have an account?Sign in to comment
Reviewers

@tacaswelltacaswelltacaswell approved these changes

@dstansbydstansbydstansby approved these changes

Assignees
No one assigned
Labels
Release criticalFor bugs that make the library unusable (segfaults, incorrect plots, etc) and major regressions.
Projects
None yet
Milestone
v3.9.0
Development

Successfully merging this pull request may close these issues.

5 participants
@timhoffm@tacaswell@rcomer@story645@dstansby

[8]ページ先頭

©2009-2025 Movatter.jp