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

Check parameter type for legend(labels)#16864

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
QuLogic merged 1 commit intomatplotlib:masterfromtimhoffm:legend-labels-str2
Apr 16, 2020

Conversation

timhoffm
Copy link
Member

PR Summary

We supportlegend(labels). However, it may happen that users think they can just pass in a list of handles (legend(handles)). This PR checks for the type of the first positional variable and raises a ValueError when the passed list contains an Artist.

Closes#16567. Supersedes#16771.

@@ -33,10 +33,10 @@

fig, ax = plt.subplots()
lines = ax.plot(data)
ax.legend(lines)
Copy link
MemberAuthor

@timhoffmtimhoffmMar 21, 2020
edited
Loading

Choose a reason for hiding this comment

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

The check caught this wrong usage in an example. 🤓

Fixed to do what is claimed to be done above (" If you simply plot the lines
want a legend item to show up for each one. If you simply plot the lines
and callax.legend(), you will get the following:").

Overall the example is only semi-instructive. I will keep a rewrite for a separate PR. I just made the minimal change here to make it correct and consistent.

Choose a reason for hiding this comment

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

I wouldn't call it wrong per se. If you remove the argument, no legend will be there at all - which is not what the example wants to convey. It should also trigger a warning like "No handles with labels found" or similar?

Copy link
MemberAuthor

Choose a reason for hiding this comment

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

Maybe not technically wrong, but carelessly written and misleading. See my comment below.

@timhoffmtimhoffm added this to thev3.3.0 milestoneMar 21, 2020
@ImportanceOfBeingErnest
Copy link
Member

Given that the example showed passing artists in as only argument, I think one would consider this supported API and therefore deprecate with a warning instead of raising an error.

@timhoffm
Copy link
MemberAuthor

timhoffm commentedMar 21, 2020
edited
Loading

IMHO the example is careless and misleading (sorry if the author is here. No personal offence meant).

  • It does not make sense to pass all lines explicitly if one does not want to show them. - It might be ok to just demonstrate what a simpleax.legend() does, but that's not great either.
  • Given valid usecaseslegend(lines, labels) andlegend(labels), it's misleading to dolegend(lines) where the line objects are converted to str and used as labels.
  • This also gives the false impression that the marker lines in the legend are connected to the displayed names. But that's more coincidence. If you had added another artist before, the string of the firstline would be written to that artist in the legend.
  • Even though technically acceptedlegend(lines) as used here is not a real-world usecase.Line.__str__ is not useful in a legend at all.

Given these issues, I claim that neither was it the intention of the example to announce passing artists as labels, nor are there hardly any people out there using it in the real world. Even though I'm normally defensive about API changes, I do not consider it necessary to do an API-depreaction dance in this case.

@QuLogicQuLogic merged commit53faed9 intomatplotlib:masterApr 16, 2020
@timhoffmtimhoffm deleted the legend-labels-str2 branchApril 16, 2020 05:51
Sign up for freeto join this conversation on GitHub. Already have an account?Sign in to comment
Reviewers

@ImportanceOfBeingErnestImportanceOfBeingErnestImportanceOfBeingErnest left review comments

@QuLogicQuLogicQuLogic approved these changes

@anntzeranntzeranntzer approved these changes

Assignees
No one assigned
Projects
None yet
Milestone
v3.3.0
Development

Successfully merging this pull request may close these issues.

Let legend get the handles from the provided objects if not specified explicitly.
4 participants
@timhoffm@ImportanceOfBeingErnest@QuLogic@anntzer

[8]ページ先頭

©2009-2025 Movatter.jp