-
-
Notifications
You must be signed in to change notification settings - Fork 8.1k
ENH: introduce wedge_labels parameter for pie
#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 our terms of service and privacy 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
e5db08a to
925704e
Compare
|
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? |
|
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 🤔 |
|
Also note that The original proposal in #19338 was for |
|
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. |
|
I am also wondering if it’s worth factoring out a public |
|
What may be a good idea is a private |
|
OK let me see if I can summarise the current thinking:
Footnotes |
|
Good summary! This is one 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 parameter 1). 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
|
|
Completely green fielding this issue a I'm pretty ignorant about |
|
I would prioritise formatting fractions over absolute values:
Edit: I guess you could detect whether the format string contains "%}". If it does, give it fractions. If not give it absolute values. |
Yes, I've briefly thought about that too, but it's too much magic 🪄 . There's a standard expectation how |
|
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%)' |
Great idea. Add this format-string as acceptable input for The fundamental question is: Do we want to go through the hassle of repurposing |
|
I am in favour of making
|
|
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 |
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 |
Not easily. See #29152 (comment) for the issues I see with |
|
What would be the default for |
|
As a follow up, could |
|
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 of Container 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. |
|
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. |
|
Because when regarding Pie as a semantic entity, you can e.g. add a |
|
Ok that's fair enough. I guess the wedges are not independent in that if you change one you need to change the others. |
|
OK, but can we say the initial implementation of |
|
Sure. I think this would be more or less sufficient for a start. class PieContainer:
def __init__(patches, texts, autotexts=None):
self._patches = patches
self._texts = texts
self._autotexts = autotexts
def __iter__(self):
# needed to support unpacking into a tuple for backward compatibility
return (
(self._patches, self._texts)
if self._autotexts is None
else (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 |
|
There seems to be a consensus for the |
piepie
piepie
|
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 a rotate_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 the labels and autopct parameters from the examples in favour of either wedge_labels or a separate call to |
lib/matplotlib/axes/_axes.py
Outdated
| fracs = x | ||
| if labels is None: | ||
| labels = [''] * len(x) | ||
| elif labeldistance is False: |
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 introducing False. AFAICS
| elif labeldistance is False: | |
| elif labeldistance == 1.1: |
would have the same effect. The only difference is that False allows to suppress the deprecation warning by setting labeldistance=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 the Rules. 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.
| if wedge_labels is not None: | ||
| self.pie_label(pc, wedge_labels, distance=wedge_label_distance, | ||
| textprops=textprops) | ||
|
|
||
| elif labeldistance is None: | ||
| # Insert an empty list of texts for backwards compatibility of the | ||
| # return value. | ||
| pc.add_texts([]) | ||
| else: | ||
|
|
||
| if labeldistance is not None: |
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 prohibit wedge_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.
Since wedge_labels is new, anybody who sets that could also adapt to labeldistance=None; and if needed use pie_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.

PR summary
The proposal evolved through the below discussion. The idea is to introduce a new wedge_labels parameter to
piewhich supersedes autopct, via its use of thestr.formatmethod, but it can also take a list of labels consistent with the existing labels parameter.pie_labelmethod introduced in ENH: introduce PieContainer and pie_label method #30733.None(which means labels are only used for the legend). Later remove this parameter altogether - direct to use wedge_labels instead.pie#29152 (comment).autopctin favour ofwedge_labels.PR checklist