Skip to content

Fix boxplot legend entries #27568

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

Closed
wants to merge 0 commits into from
Closed

Fix boxplot legend entries #27568

wants to merge 0 commits into from

Conversation

saranti
Copy link
Contributor

@saranti saranti commented Dec 26, 2023

PR summary

Work on this is continued in #27711

Closes #20512. Legend entries in boxplots are now shown as patches instead of thin lines. As explained in #20512 (comment), legend entries were being generated for the box and also for the whiskers, caps, and outliers. This patch prevents entries being created for every artist except the box itself by applying the label="_nolegend_" argument for those artists.

PR checklist

Copy link
Member

@story645 story645 left a comment

Choose a reason for hiding this comment

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

I agree w/ @anntzer about the full solution being to create a BoxplotLegendHandler, but would be great if this is good enough to fix things. Can you please add a test that it does?

@@ -4319,14 +4319,14 @@ def do_patch(xs, ys, **kwargs):
do_box = do_patch if patch_artist else do_plot
boxes.append(do_box(box_x, box_y, **box_kw))
# draw the whiskers
whiskers.append(do_plot(whis_x, whislo_y, **whisker_kw))
whiskers.append(do_plot(whis_x, whishi_y, **whisker_kw))
whiskers.append(do_plot(whis_x, whislo_y, **whisker_kw, label="_nolegend_"))
Copy link
Member

Choose a reason for hiding this comment

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

This should be done as

whisker_kw.setdefault('label', '_nolegend_)

or similar where we generate/accept these dictionaries. As proposed this will fail with a TypeError for any user who is currently passing label though.

Copy link
Member

@story645 story645 left a comment

Choose a reason for hiding this comment

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

might also want to confirm that the patch properties are as expected, eg. color.

What happens if you set a label on those elements?

@saranti
Copy link
Contributor Author

saranti commented Dec 30, 2023

might also want to confirm that the patch properties are as expected, eg. color.

What happens if you set a label on those elements?

Setting a label to an element like whiskers or caps will revert the next 2 handles into lines. In this case, bp0's whiskers are set a label and color (with @tacaswell's changes). I'll have to dig deeper into this.

Figure_1

@tacaswell
Copy link
Member

We have a labels parameter to boxplot (and it is in the datastructure that goes into bxp) that we currently only use for setting tick labels if manageticks. I think we should also use that to set the label on the boxes.


The fundamental problem is that calling legend with a list of strings is a really (really) brittle API. If it did not exist and was proposed now we would almost certainly reject it. Unfortunately it is too widely used and deprecating it is also off the table so we just have to live with it.

The brittleness comes from the logic of "I have been given N strings, let me find the first N artists in the axes that I know how to make legend entries for and use those!". By setting '_nolabel_' we exclude the lines etc from the "do you want to be in the legend?" search and hence "fix" the problem. This is also the reason why the way it was broken changed between versions. We recently moved from keeping lists of artists by type to a single list of artists so the details about the exact order of iteration of children of different types at the same zorder changed (now strictly the order added, previously runs of same type).

The other ways of making legends (either passing both artists and strings ("use these labels for those artists"), passing a list of artists ("ask these artists what their label should be and then put in legend"2 or calling with no arguements ("find anything that has a suitable label and put it in the legend") are all significantly better APIs (easier to understand and not brittle).

If the user set a label on the whiskers, it is presumably because they are doing ax.legend() or for some other reason accessing obj.get_label() and we definitely should not break them to support the list-of-string use case. If we set the label on the patches then ax.legend() will "just work" and let users get away from the very sub-optimal way they have to do it now.

@tacaswell tacaswell added this to the v3.9.0 milestone Jan 3, 2024
@saranti
Copy link
Contributor Author

saranti commented Jan 6, 2024

We have a labels parameter to boxplot (and it is in the datastructure that goes into bxp) that we currently only use for setting tick labels if manageticks. I think we should also use that to set the label on the boxes.

I've implemented this idea so now we can call ax.legend() with no parameters to get a properly labelled legend. Changing other labels will still add them to the legend but at least now they're labelled correctly.

@tacaswell
Copy link
Member

Talked about this on the call.

We like the implementation, but it needs a rebase.

@timhoffm
Copy link
Member

@saranti Did you mean to close this?

@saranti
Copy link
Contributor Author

saranti commented Jan 27, 2024

@saranti Did you mean to close this?

No, I only meant to rebase. Can you re-open it please? 😃

Continued on PR #27711

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Bad boxplot legend entries
6 participants