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

Allow option to display absolute values for pie chart #19338#25757

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

Closed
codebreakymefix wants to merge2 commits intomatplotlib:mainfromcodebreakymefix:main
Closed

Conversation

codebreakymefix
Copy link

@codebreakymefixcodebreakymefix commentedApr 24, 2023•
edited
Loading

PR Summary

Adds a parameter 'wedgelabels' to pie() for the user to display absolute or percent values in the wedges. Renamed 'autopct' to "wedgelabelsfmt" as the user requested.

closes#19338

PR Checklist

Linked Issue

  • Added "closes #0000" in the PR description to link it to the original issue.

Documentation and Tests

  • Has pytest style unit tests (andpytest passes)
  • Documentation is sphinx and numpydoc compliant (the docs shouldbuild without error).
  • New plotting related features are documented with examples.

Release Notes

  • New features are marked with a.. versionadded:: directive in the docstring and documented indoc/users/next_whats_new/
  • API changes are marked with a.. versionchanged:: directive in the docstring and documented indoc/api/next_api_changes/
  • Release notes conform with instructions innext_whats_new/README.rst ornext_api_changes/README.rst

codebreakymefixand others added2 commitsApril 23, 2023 20:59
chart#19338Added a paramter wedgelabels to the pie() so the user can choose ifthey values are absolute or percentages. Renamed autopct withwedgelabelsfmt as the user requested.
Copy link

@github-actionsgithub-actionsbot left a comment

Choose a reason for hiding this comment

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

Thank you for opening your first PR into Matplotlib!

If you have not heard from us in a while, please feel free to ping@matplotlib/developers or anyone who has commented on the PR. Most of our reviewers are volunteers and sometimes things fall through the cracks.

You can also join uson gitter for real-time discussion.

For details on testing, writing docs, and our review process, please seethe developer guide

We strive to be a welcoming and open project. Please follow ourCode of Conduct.

Copy link
Member

@oscargusoscargus left a comment•
edited
Loading

Choose a reason for hiding this comment

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

Thanks for this!

It is not clear to me how this works in practice. Right now, bothautofmt andwedgelabelsfmt does the same thing? If so, why should we have both? If not, how can we deprecateautofmt later?

If it is only a matter of a "bad" name, then I think it is better to just have one name and use the decorator@_api.rename_parameter("3.8", "autofmt", "wedgelabelsfmt").

However, I would wait for additional input before going all in on these changes. (New arguments after* is a requirement though.)

Edit: after reading the issue, I think it should be quite OK to add the decorator as well.

wedgeprops=None, textprops=None, center=(0, 0),
autopct=None,wedgelabels=None, wedgelabelsfmt=None, pctdistance=0.6,
shadow=False, labeldistance=1.1, startangle=0, radius=1,
counterclock=True,wedgeprops=None, textprops=None, center=(0, 0),
frame=False, rotatelabels=False, *, normalize=True, hatch=None):
Copy link
Member

Choose a reason for hiding this comment

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

The new arguments must be added after the* in case someone is using positional arguments.

@story645
Copy link
Member

I get the need and support supporting it, but I'm very iffy on coupled keywords - (wedgelabels, wedgelabelfmt) and would prefer an 'absolutefmt' keyword w/ the instruction that you cannot use both. Either way, I think wedgelabels is confusing cause labels is also confusing and we're really labeling the categories and amounts (maybe intertior labels?)

And, as mentioned in the checklist, once settled this will need an example (I suggest adding it to the pie demo).

@rcomer
Copy link
Member

Hi@codebreakymefix are you still interested in working in this? It looks like maybe you were waiting for reviewers to reach a consensus.

I think I agree with@story645 on this that the proposedwedgelabels could be confusing. As thepie example shows, the currentlabels can be put on the wedge and the automatic percentage labels outside.

Perhapsautolabels andautolabelfmt rather thanwedge… might work? Or I also like@story645’s suggestion of an “absolute” format keyword that can’t be passed together withautopct.

@rcomerrcomer added the status: needs comment/discussionneeds consensus on next step labelSep 29, 2023
@rcomer
Copy link
Member

Discussion on parameter name also needs to consider thepctdistance parameter.

story645 reacted with thumbs up emoji

@codebreakymefixcodebreakymefix closed this by deleting the head repositoryMay 7, 2024
Sign up for freeto join this conversation on GitHub. Already have an account?Sign in to comment
Reviewers

@github-actionsgithub-actions[bot]github-actions[bot] left review comments

@oscargusoscargusoscargus requested changes

Assignees
No one assigned
Projects
Status: Needs decision
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Allow option to display absolute values for pie chart
5 participants
@codebreakymefix@story645@rcomer@oscargus@QuLogic
[8]ページ先頭
©2009-2025 Movatter.jp