Uh oh!
There was an error while loading.Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork8.1k
ENH: introduce wedge_labels parameter forpie#29152
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
base:main
Are you sure you want to change the base?
Conversation
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
rcomer commentedNov 17, 2024 • 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.
e5db08a to925704eComparetimhoffm commentedNov 17, 2024
If we are touching the existing API, should we reconsider more fundamentally? Currently,
Some observations:
What we fundamentally need to keep:The two sets of annotations must stay. We'd loose functionality if we allowed only one label. Also, a generalization to more than two sets of annotations does not make sense. Side-note: If I was designing this from scratch, I'd likely focus on location Therefore, I propose to
Does that sound reasonable? |
rcomer commentedNov 17, 2024 • 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.
I am not sure about Edit: though I guess by supporting the list of strings we allow the users to format those absolute values however they like within a list comprehension before passing them in 🤔 |
rcomer commentedNov 18, 2024
Also note that The original proposal in#19338 was for |
timhoffm commentedNov 18, 2024
What I've missed in my above thought is that I'm trying to find a consistent way to map the option space:
There are two ways for structuring:
IMHO the "absolute values" request comes from the fact that the inner label is too specific in that it only handles percent. There's no API to get arbitrary text in there. A minimal green-field approach would be I therefore revise my above proposal to the following minimal change:
One can later decide whether both of the labels parameters should grow some formatting / function capability. |
timhoffm commentedNov 18, 2024 • 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.
rcomer commentedNov 18, 2024
I am also wondering if it’s worth factoring out a public |
timhoffm commentedNov 18, 2024
What may be a good idea is a private |
rcomer commentedNov 18, 2024 • 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.
OK let me see if I can summarise the current thinking:
Footnotes |
timhoffm commentedNov 18, 2024 • 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.
Good summary! This isone viable way to make the API more consistent.
formatting is a bit tricky because you have to decide whether the inputs should be absolute values or fractions. You cannot have both (at least not without an extra parameter1). I'm inclined to expect absolute values by default, and that would still not give a replacement for autopct. We could decide to go with relative values, but it feels a bit unintuitive The function approach is a bit better, because you could support two signatures Alternative: pie_label()I've thought about this a bit more. The idea is fundamentally reasonable. On the plus side, we get better decoupling and full control over each label set - currently Problem 1: AFAICS, we cannot retrieve the underlying data value from a wedge. Therefore no external function can create labels based on the output of If feel this is less good than the summarized approach. Footnotes
|
jklymak commentedNov 18, 2024
Completely green fielding this issue a I'm pretty ignorant about |
rcomer commentedNov 19, 2024 • 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.
I would prioritise formatting fractions over absolute values:
Edit: I guess youcould detect whether the format string contains "%}". If it does, give it fractions. If not give it absolute values. |
timhoffm commentedNov 19, 2024 • 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.
Yes, I've briefly thought about that too, but it's too much magic 🪄 . There's a standard expectation how |
rcomer commentedNov 19, 2024 • 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.
Named placeholders would give a lot more flexibility... In [2]:'{abs:d}'.format(abs=42,frac=0.8)Out[2]:'42'In [3]:'{frac:.1%}'.format(abs=42,frac=0.8)Out[3]:'80.0%'In [4]:'{abs:d} ({frac:.0%})'.format(abs=42,frac=0.8)Out[4]:'42 (80%)' |
timhoffm commentedNov 20, 2024
Great idea. Add this format-string as acceptable input for The fundamental question is: Do we want to go through the hassle of repurposing |
rcomer commentedNov 20, 2024
I am in favour of making
|
timhoffm commentedNov 20, 2024 • 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.
Side note on naming: I've briefly pondered whether in would make sense to bikeshed Edit: The only other alternative that may be worth considering is to make a single instead of |
story645 commentedNov 20, 2024 • 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.
I really like this proposal b/c it removes the semantic issue of being able to position inner/outer label anywhere, and it gives people the flexibility to have as many sets of labels as they want. Though could this also be the external function |
timhoffm commentedNov 20, 2024
Not easily. See#29152 (comment) for the issues I see with |
rcomer commentedNov 20, 2024
What would be the default for |
timhoffm commentedOct 22, 2025
Yes. A split into two PRs is reasonable. We could split also in another way: (1) Modify the |
story645 commentedOct 22, 2025
As a follow up, could |
timhoffm commentedOct 22, 2025 • 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.
Yes Re: Design - I believe we need some container as the return type of We currently have We should end up with so that the above stays valid, i.e. it supports unpacking I'm slightly unclear whether we want a pure container in the sense ofContainer or a semantic artist like StepPatch. The difference being that Containers basically just bundle some artists, but semantic artists, add special logic and meaning. Maybe it's also both here. |
jklymak commentedOct 22, 2025
Why wouldn't you return a list of The point being you could just label some of the wedges if you just pass a sublist of wedges. |
timhoffm commentedOct 22, 2025
Because when regarding Pie as a semantic entity, you can e.g. add a |
jklymak commentedOct 22, 2025
Ok that's fair enough. I guess the wedges are not independent in that if you change one you need to change the others. |
rcomer commentedOct 23, 2025
OK, but can we say the initial implementation of |
timhoffm commentedOct 23, 2025 • 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.
Sure. I think this would be more or less sufficient for a start. classPieContainer:def__init__(patches,texts,autotexts=None):self._patches=patchesself._texts=textsself._autotexts=autotextsdef__iter__(self):# needed to support unpacking into a tuple for backward compatibilityreturn ( (self._patches,self._texts)ifself._autotextsisNoneelse (self._patches,self._texts,self._autotexts) ) One could add more methods to increase backward-compatibility with tuples, but I suspect basically all use cases will immediately unpack ( You don't even have to make the data public - and I wouldn't right now. It's likely that we'll want to store multiple sets of labels, and then |
rcomer commentedOct 25, 2025
There seems to be a consensus for the |
piepiepiepiercomer commentedNov 29, 2025 • 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.
This one is now ready for review again, but should not be merged before the v3.11 branch is created. I am happy that this is much cleaner than the previous iteration, which was keeping track of too many things within the Unlike the previous iteration, I have not introduced arotate_wedge_labels parameter. I think rotation is not a particularly common case, and anyone who does want it can use the I have removed all uses of thelabels andautopct parameters from the examples in favour of eitherwedge_labels or a separate call to |
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
lib/matplotlib/axes/_axes.py Outdated
| fracs=x | ||
| iflabelsisNone: | ||
| labels= ['']*len(x) | ||
| eliflabeldistanceisFalse: |
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.
Is there a benefit in introducingFalse. AFAICS
| eliflabeldistanceisFalse: | |
| eliflabeldistance==1.1: |
would have the same effect. The only difference is thatFalse allows to suppress the deprecation warning by settinglabeldistance=1.1 explicitly. But that only buys you some time as the parameter will be removed eventually. You could either ignore the warning, or switch to the newer methods, or if needed version-gate your code.
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.
Buying time is part of the point, since I think we agreed somewhere that functionality should not be deprecated until its alternative has been available for at least one meso version. Though now I look, that discussion did not make it into theRules. I note that at#29152 (comment) you were keen to do this slowly.
Also it would be strange to me that passing 1.1 gives me a warning when any other number does not.
| ifwedge_labelsisnotNone: | ||
| self.pie_label(pc,wedge_labels,distance=wedge_label_distance, | ||
| textprops=textprops) | ||
| eliflabeldistanceisNone: | ||
| # Insert an empty list of texts for backwards compatibility of the | ||
| # return value. | ||
| pc.add_texts([]) | ||
| else: | ||
| iflabeldistanceisnotNone: |
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.
Do we want to prohibitwedge_labels is not None and labels is not None and labeldistance is not None. This would add two sets of labels. Currently, and after deprecation, we'll only add one set of labels.
Sincewedge_labels is new, anybody who sets that could also adapt tolabeldistance=None; and if needed usepie_label to add the second set of labels.
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.
Yes, good point.
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.
Done.

Uh oh!
There was an error while loading.Please reload this page.
PR summary
The proposal evolved through the below discussion. The idea is to introduce a newwedge_labels parameter to
piewhich supersedesautopct, via its use of thestr.formatmethod, but it can also take a list of labels consistent with the existinglabels parameter.pie_labelmethod introduced inENH: introduce PieContainer and pie_label method #30733.None(which meanslabels are only used for the legend). Later remove this parameter altogether - direct to usewedge_labels instead.pie#29152 (comment).autopctin favour ofwedge_labels.PR checklist