Skip to content
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

abeltavares
Copy link
Contributor

@abeltavares abeltavares commented Apr 14, 2024

@abeltavares abeltavares force-pushed the BUG/Series.plot-does-not-respect-ylabel-argument branch 2 times, most recently from 8b97776 to 48c3abe Compare April 14, 2024 16:54
@mroeschke mroeschke added the Visualization plotting label Apr 15, 2024
@abeltavares abeltavares force-pushed the BUG/Series.plot-does-not-respect-ylabel-argument branch 2 times, most recently from b494748 to 99bc4b3 Compare April 18, 2024 23:13
Copy link
Member

@WillAyd WillAyd left a 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

@@ -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
Copy link
Member

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?

Copy link
Contributor Author

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 ylabelparameter defined by the user for the chart is already being tested as implemented on (#34223).
So I think this solves the issue.

Copy link
Member

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

Copy link
Member

@WillAyd WillAyd left a 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?

@@ -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
Copy link
Member

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

@abeltavares abeltavares force-pushed the BUG/Series.plot-does-not-respect-ylabel-argument branch from 99bc4b3 to 6b39485 Compare April 27, 2024 23:25
@abeltavares abeltavares force-pushed the BUG/Series.plot-does-not-respect-ylabel-argument branch from 6b39485 to 0dd3524 Compare April 29, 2024 08:08
Copy link
Member

@WillAyd WillAyd left a comment

Choose a reason for hiding this comment

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

lgtm. @mroeschke

@mroeschke mroeschke added this to the 3.0 milestone Apr 29, 2024
@mroeschke mroeschke merged commit 72d0612 into pandas-dev:main Apr 29, 2024
46 checks passed
@mroeschke
Copy link
Member

Thanks @abeltavares

pmhatre1 pushed a commit to pmhatre1/pandas-pmhatre1 that referenced this pull request May 7, 2024
…-dev#58254)

Co-authored-by: Abel Tavares <abel.tavares@ctw.bmwgroup.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

BUG: Series.plot(kind="pie") does not respect ylabel argument
3 participants