Uh oh!
There was an error while loading.Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork7.9k
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
Uh oh!
There was an error while loading.Please reload this page.
Conversation
Uh oh!
There was an error while loading.Please reload this page.
@@ -33,10 +33,10 @@ | |||
fig, ax = plt.subplots() | |||
lines = ax.plot(data) | |||
ax.legend(lines) |
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.
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.
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.
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?
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.
Maybe not technically wrong, but carelessly written and misleading. See my comment below.
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 commentedMar 21, 2020 • 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.
IMHO the example is careless and misleading (sorry if the author is here. No personal offence meant).
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. |
PR Summary
We support
legend(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.