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?
Uh oh!
There was an error while loading.Please reload this page.
Conversation
| # The use of float32 is "historical", but can't be changed without | ||
| # regenerating the test baselines. | ||
| x=np.asarray(x,np.float32) | ||
| x=np.asarray(x) |
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.
Casting to float here meant that we couldn't use "%d" for the format string, which is the most obvious thing to use if your input values are int. No tests failed for me locally. I note that tolerances were recently added for numpy v2:
matplotlib/lib/matplotlib/tests/test_axes.py
Lines 5947 to 5950 in183b04f
| # Note: The `pie` image tests were affected by Numpy 2.0 changing promotions | |
| # (NEP 50). While the changes were only marginal, tolerances were introduced. | |
| # These tolerances could likely go away when numpy 2.0 is the minimum supported | |
| # numpy and the images are regenerated. |
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 |
rcomer commentedOct 19, 2025
I believe this one is now ready for review. I have updated the PR summary with the current proposal. Note I am targeting v3.12. |
timhoffm commentedOct 21, 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.
Sorry for letting this slip. I will do a review soon. Looking at this with some distance, I'm undecided whether On the one hand, a single parameter is generic and efficient. On the other hand, it's getting a bit complex and almost ambiguous: @rcomer since you have worked with this in detail, what do you think? Are we trying to be too clever here? |
rcomer commentedOct 21, 2025
Thanks@timhoffm. You didn't let it slip - it was me who had it in draft for most of the year! Thanks to your suggestion at#29152 (comment), we detect the difference between format strings and plain strings so the ambiguity is properly handled: plt.pie([5,2],wedge_labels=["spam","eggs"]) ![]() plt.pie([5,2],wedge_labels=["{absval:d}","{frac:.0%}"]) plt.pie([5,2],wedge_labels=["{absval:d}","{frac:.0%}"],wedge_label_distance=[0.6,1.1]) ![]() So I do not think there is any technical problem with usingwedge_labels, we only need to decide from an API point of view whether it's intuitive enough. I know@story645 was keen on the singlewedge_labels parameter. |
timhoffm commentedOct 21, 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.
Exactly. We have the following use cases (Note: i use
ax.pie(values,wedge_labels=fractions)ax.pie(values,inner_labels=fractions)
ax.pie(values,wedge_labels=fractions,wedge_label_distance=1.1)ax.pie(values,outer_labels=fractions)
ax.pie(values,wedge_labels=[fractions,names],wedge_label_distance=[0.6,1.1])ax.pie(values,inner_labels=fractions,outer_labels=names) 3b. or if we add some magic, the two wedge_labels case could automatically result in default distances ax.pie(values,wedge_labels=[fractions,names])ax.pie(values,inner_labels=fractions,outer_labels=names)
ax.pie(values,wedge_labels=[fractions,names],wedge_label_distance=[0.5,1.3])ax.pie(values,inner_labels=fractions,outer_labels=names,inner_label_distance=0.5,outer_label_distance=1.3) Looking at the code, I did a personal subjective usability rating - feel free to do yours.
Overall, Update: Included the options |
story645 commentedOct 21, 2025
I don't like them b/c we don't force any semantic constraints on them, which means realistically you can use either one for the other case. Consistently the "why are there 3 ways to do this?" is what creates confusion. If I'm setting multiple labels, I like being able to use two parameters over 4. Ideally, I'd love a (label, distance) tuple to remove coupled keywords, but my guess is that gets very messy very fast from a parsing pov. |
jklymak commentedOct 21, 2025
I didn't read all of the above, but did you consider ax.label_wedges or some such? It has a precedent in contour and clabel, and multiple labels could easily be implemented by multiple calls. |
timhoffm commentedOct 21, 2025
See#29152 (comment). |
jklymak commentedOct 21, 2025
It seems the only issue is making the wedges store info about their value? That doesn't seem to be a huge lift. |
timhoffm commentedOct 21, 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.
I think you'd need a new Artist (Wedge subclass) for that as wedges are only geometric shapes and do not sore information in some absolute value. That is some work but doable. There's a second issue in that the API changes gets larger. currently, |
timhoffm commentedOct 21, 2025
Well, there's semantics though the defaults. Yes, you can override this by intentionally choosing other parameters. But even then, I would not worry too much about this. There's still one canonical way to do each use case. Yes you could do |
story645 commentedOct 21, 2025
Sure, but here we're intentionally giving users 2 parameters that functionally behave in the same way, which makes one of them feel redundant. |
jklymak commentedOct 21, 2025
Fair enough. Personally, I'd live with the mild inconsistency and allow extra labels with a call to |
timhoffm commentedOct 22, 2025
I've updated my evaluationabove with these variants, and it indeed seems most usable to have a simple |
rcomer commentedOct 22, 2025
Just to make sure I’ve understood, if we add the It did feel a little clunky adding |
rcomer commentedOct 22, 2025
If we do go that route, I propose to split the change into two PRs: PR1. Introduce PR2. Introduce the conveniencewedge_labels parameter. Make the deprecations and discouragement for existing label functionality as listed above. PR1 would close the original issue, so we could even decide to stop there if we want. |
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 |



Uh oh!
There was an error while loading.Please reload this page.
PR summary
Closes#19338
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. The current implementation mostly follows the proposal agreed at#29152 (comment)..format(absval=absval, frac=frac), or a sequence of those. See the whatsnew entry and its linked example for details.absvalrather thanabsagreed below, since I do not think we should use the name of a builtin function. This and other names can of course be bikeshedded further.wedge_label_distancemust be a sequence of the same length. Otherwise error.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. If passed, internally incorporate it into thewedge_labelhandling.PR checklist