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

ENH: introduce PieContainer and pie_label method#30733

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

Open
rcomer wants to merge1 commit intomatplotlib:main
base:main
Choose a base branch
Loading
fromrcomer:pie_label

Conversation

@rcomer
Copy link
Member

PR summary

This PR introduces a newpie_label method for adding labels to the wedges of existing pie charts, following discussion in#29152. Similarly toBarContainer, we introduce aPieContainer as the new return value ofpie. This contains theWedge patches and the numbers thatpie_label requires for automatically labelling the wedges with their values and/or fractions.

Closes#19338

A few things I’m not sure about:

  • So far I have added very few new tests, since most of thepie_label functionality should be covered by existingpie tests that use thelabels and/orautopct parameters. Is is better to duplicate this coverage?
  • ShouldPieContainer inherit fromContainer as I have currently done? It feels a bit wrong to me as I have also overwritten the__iter__ method to provide backwards compatibility on the return value ofpie. So we do not get the standardContainer behaviour that looping through it gives you the patches one by one. OTOH it is nice to get theremove method for free.
  • Shouldpie_label accept a function for thelabels parameter, similar to theautopct parameter ofpie? Initially I thought no, because I do not know of a use case and the only example I found in the docs was the bakery one, which can now use the format string. However, supporting a function would allow us to move more of theautopct handling intopie_label, and remove the duplicate handling for the "usetex" case.

PR checklist

@github-actionsgithub-actionsbot added topic: pyplot API Documentation: examplesfiles in galleries/examples Documentation: APIfiles in lib/ and doc/api labelsNov 7, 2025
rotate : bool, default: False
Rotate each label to the angle of the corresponding slice if true.
alignment : {'center', 'outer', 'auto'}, default: 'auto'
Copy link
MemberAuthor

@rcomerrcomerNov 7, 2025
edited
Loading

Choose a reason for hiding this comment

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

'center' is how theautopct labels ofpie are aligned and 'outer' is how thelabels ofpie are aligned. We had some discussion about the horizontal alignment at#29152 (comment) and#29152 (comment), but I did not notice the extra complication for vertical alignment withrotate until I was writing the example.autopct does not have an associated rotate option, but I can report that rotated labels that were centred horizontally but not centred vertically looked bad!

timhoffm reacted with thumbs up emoji
@rcomerrcomer marked this pull request as ready for reviewNovember 7, 2025 19:34
@timhoffm
Copy link
Member

timhoffm commentedNov 8, 2025
edited
Loading

  • ShouldPieContainer inherit fromContainer as I have currently done?

Container falls short for our needs here. It is basically a tuple of artists withremove() propagation and a CallbackRegistry (which AFAICS we nowhere use). That simple tuple approach does not suit the more general case that we have not just one type sequence of Artists. In fact, onlyBarContainer really fits in. AlreadyErrorbarContainer andStemContainer have to work around the limitation using tuples of tuples, which is not a good API.

I think we need to come up with a more generic concept. For a start, I'd say make your own type (i.e. copy the remove method) and define whatever minimal API is reasonable. If we know what kind of API we want to have based on this example it's easier to design a more generalArtistCollection orCompoundArtist.

On a side-note we want this to not only be pure collections, but be a semantic entity, likeStepPatch, i.e. there may be (not necessarily from the start) semantic functions, that non-trivially manipulate multiple properties of the artists; e.g. setting the pie values would need to recompute positions of all wedges and texts and update the text content.

Edit: The case is similar togrouped_bar(), where we currently also only have a provisional class with functionality and minimal guarantees.

rcomer reacted with thumbs up emoji

@timhoffm
Copy link
Member

  • So far I have added very few new tests, since most of thepie_label functionality should be covered by existingpie tests that use thelabels and/orautopct parameters. Is is better to duplicate this coverage?

If writing from scratch, I would mostly test the functionality onpie_label() directly and only ensure basic propagation frompie() topie_label(). Given there are already many tests onpie() that is good enough for a start. I would not duplicate but rather move coverage out ofpie() tests and intopie_label() tests. But that is a nice-to-have. If done, I'd rather make this a separate cleanup PR.

rcomer reacted with thumbs up emoji

@rcomerrcomerforce-pushed thepie_label branch 2 times, most recently from867bd4a tobed6636CompareNovember 9, 2025 19:53
@rcomer
Copy link
MemberAuthor

OK,PieContainer is now its own class with its ownremove method.

@rcomerrcomer marked this pull request as draftNovember 10, 2025 10:40
@rcomer
Copy link
MemberAuthor

rcomer commentedNov 10, 2025
edited
Loading

For complete backwards compatibility, we need "texts" to be returned as an empty list whenlabeldistance isNone. I think both my current and previous implementations broke that.

In draft for now while I think about how to address that. Suggestions of course welcome!

Edit: this is now addressed by the next push.

@rcomer
Copy link
MemberAuthor

rcomer commentedNov 10, 2025
edited
Loading

Apologies - just found some doc changes that I had missed before. None of the examples now unpack or index the return value.

super().__init__(lines,**kwargs)


classPieContainer:
Copy link
Member

Choose a reason for hiding this comment

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

I am unsure on the name. I'm tempted to call this justPie on the background, that I envision this will become a compound semantic artist. The "Container" puts more focus on "aggregation of parts".

In that context, I'm pondering whetherpie() could/should become essentially

defpie(self,*args,**kwargs):pie=Pie(*args,**kwargs)self.add_artist(pie)returnpie

OTOH, full semantics may be more than we can easily achieve; e.g. assume a laterpie.set_values(new_values) changes the number of wedges. How does that affect explicitly set lists of labels or wedge properties?

Recommendation: Let's stick withPieContainer for now, but make then name provisional. - It's not needed for the basic use case of

pie = ax.pie(...)ax.pie_label(pie, ....)

Copy link
Member

@timhoffmtimhoffm left a comment

Choose a reason for hiding this comment

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

I've left some minor comments.

Overall a systematic and easy-to-review PR. Thanks for the effort.🚀

rcomer reacted with heart emoji
returnslices,texts,autotexts
returnpc

defpie_label(self,container,labels,*,distance=0.6,
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
defpie_label(self,container,labels,*,distance=0.6,
defpie_label(self,container,/,labels,*,distance=0.6,

A bit unconventional, but we could makecontainer positional-only so that we can more easily rename it should the need arise.

Comment on lines +157 to +158
The ``PieContainer`` name is provisional and may change in future to reflect
development of its functionality.
Copy link
Member

Choose a reason for hiding this comment

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

This could have a more prominent styling. If you think warning is too much, I'm also ok with note

Suggested change
The``PieContainer``nameisprovisionalandmaychangeinfuturetoreflect
developmentofitsfunctionality.
..warning::
Theclassname``PieContainer``nameisprovisionalandmaychangeinfuture
toreflectdevelopmentofitsfunctionality.

# escape % (i.e. \%) if it is not already escaped
labels= [re.sub(r"([^\\])%",r"\1\\%",s)forsinlabels]
elif (nw:=len(container.wedges))!= (nl:=len(labels)):
raiseValueError(f'Found{nw} wedges but{nl} labels')
Copy link
Member

Choose a reason for hiding this comment

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

Optional: State what is required not just what is present.

Suggested change
raiseValueError(f'Found{nw} wedges but{nl} labels')
raiseValueError(
f'The number of labels ({nl}) must match the number of wedges ({nw})')

Sign up for freeto join this conversation on GitHub. Already have an account?Sign in to comment

Reviewers

@timhoffmtimhoffmtimhoffm approved these changes

Assignees

No one assigned

Labels

Documentation: APIfiles in lib/ and doc/apiDocumentation: examplesfiles in galleries/examplesNew featuretopic: pyplot API

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Allow option to display absolute values for pie chart

2 participants

@rcomer@timhoffm

[8]ページ先頭

©2009-2025 Movatter.jp