Uh oh!
There was an error while loading.Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork7.9k
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
Partly revert #27711#27780
Uh oh!
There was an error while loading.Please reload this page.
Conversation
12527a7
to8581c19
CompareI thought it was ok to propagate because there is a keyword to control if boxplot manages the ticks or not. |
timhoffm commentedFeb 13, 2024 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
Uh, ok, I did not know about that ... 🤯 I suppose, But let me rethink what the implications for usability and future API changes are. I'll put to draft. |
To elaborate more, my thinking was that users would either use do nothing (and we manage the ticks) or pass 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 commentedFeb 13, 2024 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
TL;DR: Irrespective of the tick vs. legend labelling issue, the current implementation (#27711) is not generally suitable for legend labels. After testing: In more general scenarios (tested here by adding and without re-coloring the individual bars: In particular:
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. |
oh, I agree that is broken! I thought that vector-like inputs were broadcast across boxplots.... |
rcomer commentedFeb 14, 2024 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
FWIW I think mysecond commit at#27767 would fix the first fruity case above. |
There was a problem hiding this 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.
Uh oh!
There was an error while loading.Please reload this page.
# 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() |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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
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.
There was a problem hiding this 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)`.
8581c19
to3dfcb51
Comparetimhoffm commentedFeb 14, 2024 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
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.
Fundamentally yes! Naming is an issue though, but let's discuss solutions in a separate issue. |
#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 commentedFeb 14, 2024 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
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. |
Since the only change since@tacaswell's approval was a very minor docstring wording update, I'll merge this with two approvals. |
Uh oh!
There was an error while loading.Please reload this page.
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 viahttps://matplotlib.org/stable/api/_as_gen/matplotlib.axes.Axes.bxp.html and in anexample.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 eitherset_label()
on them or pass them to the legend callax.legend(handles, labels)
.In particular ping@tacaswell@dstansby who have approved#27711.