-
-
Notifications
You must be signed in to change notification settings - Fork 18.4k
BUG: Series.plot(kind="pie") does not respect ylabel argument #58254
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
BUG: Series.plot(kind="pie") does not respect ylabel argument #58254
Conversation
8b97776
to
48c3abe
Compare
b494748
to
99bc4b3
Compare
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 - this seems like it is headed in the right direction
pandas/plotting/_matplotlib/core.py
Outdated
@@ -2090,6 +2087,7 @@ def blank_labeler(label, value): | |||
return label | |||
|
|||
idx = [pprint_thing(v) for v in self.data.index] | |||
# `label` is intentionally unused but is required for unpacking the tuple |
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 don't follow this comment - shouldn't it be used if provided? Do we have a test for pie charts that providing a label actually set it?
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.
This label
refers to the series name, that was being used to ylabel the pie as default, action that i removed now.
But still need to unpack alongside with y
.
Since we now are not using that label but still needed to provide to unpack the tuple i added that comment.
Yes, the ylabel
parameter defined by the user for the chart is already being tested as implemented on (#34223).
So I think this solves the issue.
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 would just remove the comment - I don't think it adds any value
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.
Generally looks good @mroeschke any thoughts?
pandas/plotting/_matplotlib/core.py
Outdated
@@ -2090,6 +2087,7 @@ def blank_labeler(label, value): | |||
return label | |||
|
|||
idx = [pprint_thing(v) for v in self.data.index] | |||
# `label` is intentionally unused but is required for unpacking the tuple |
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 would just remove the comment - I don't think it adds any value
99bc4b3
to
6b39485
Compare
6b39485
to
0dd3524
Compare
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.
lgtm. @mroeschke
Thanks @abeltavares |
…-dev#58254) Co-authored-by: Abel Tavares <[email protected]>
doc/source/whatsnew/vX.X.X.rst
file if fixing a bug or adding a new feature.