-
-
Notifications
You must be signed in to change notification settings - Fork 7.9k
ENH: add option to label pie charts with absolute values #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
# 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 in 183b04f
# 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. |
lib/matplotlib/axes/_axes.py
Outdated
|
||
elif absolutefmt is not None: | ||
if isinstance(absolutefmt, str): | ||
s = absolutefmt % n |
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.
It would be nice to support absolutefmt.format(n)
here as well, and similarly for autopct
. I vaguely remember seeing we had some logic somewhere that could distinguish between old and new style format strings and apply them appropriately. Did I dream that? If not, could someone remind me where I saw it?
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.
I did not dream it
matplotlib/lib/matplotlib/cbook.py
Lines 2369 to 2377 in 183b04f
def _auto_format_str(fmt, value): | |
""" | |
Apply *value* to the format string *fmt*. | |
This works both with unnamed %-style formatting and | |
unnamed {}-style formatting. %-style formatting has priority. | |
If *fmt* is %-style formattable that will be used. Otherwise, | |
{}-formatting is applied. Strings without formatting placeholders | |
are passed through as is. |
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 |
Is there contributor guidance somewhere (or an example) for documenting a pending deprecation? |
Belatedly I realised there is some difference in handling between the current matplotlib/lib/matplotlib/axes/_axes.py Lines 3401 to 3407 in 677d990
OTOH the automatic labels are just aligned to the centre. matplotlib/lib/matplotlib/axes/_axes.py Lines 3430 to 3433 in 677d990
Do we want to make horizontalalignment change based on whether Do we want to support the Apologies, I should have spotted these things much earlier. |
https://matplotlib.org/devdocs/devel/api_changes.html Please let me know what's missing from there and how we can make it clearer that that's where those docs are |
Thanks - I am aware of that page and it's great for how to deprecate something in the next meso release. However, I think we recently agreed that something should not be deprecated unless the alternative has been available for at least one meso release. So if we introduce Edit: never mind I found an example. |
I like this - it would be good to revisit once I have something working with the simpler current logic.
I think either way internally I build a list of lists, so it should be pretty easy to switch later if we decide to go that way. For now I will proceed with |
Currently the fontsize used for
Obvious options
We could also make textprops more flexible to take a dictionary for each set of labels, either now or as a future enhancement. |
Still very WIP. I only pushed so that the branch is backed up! |
Thanks! That means this information is missing from that guide and it should be updated to include it there. |
Definitely a fan of broadcast all the things, but depending on your bandwidth I agree w/ you on 1. That'll probably produce the cleanest labels by default. |
I’d intuitively go with that
If you want to look for more analogies, check |
c24c7ad
to
38363df
Compare
Thinking again about the labeldistance parameter, instead of
we could do
This gets us there in one less step and one less release. Downstream can silence the immediate deprecation warning by just passing |
There is a test case that passes Do we still need to support cases that this test represents? If so, should we go back to |
For now I have modified I'm also generally a bit concerned about how much new code there is here for a relatively small feature. |
PR summary
Closes #19338 following @story645's suggestion at #25757 (comment) to introduce a new absolutefmt parameter to label pie charts with their original input values. Rename pctdistance to autodistance as this parameter may now be used for either the percentage or absolute value labels.
New parameter names of course open to discussion!
PR checklist