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 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

Draft
rcomer wants to merge1 commit intomatplotlib:main
base:main
Choose a base branch
Loading
fromrcomer:pie-fmt

Conversation

@rcomer
Copy link
Member

@rcomerrcomer commentedNov 17, 2024
edited
Loading

PR summary

Closes#19338

The proposal evolved through the below discussion. The idea is to introduce a newwedge_labels parameter topie which supersedesautopct, via its use of thestr.format method, 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).

  • Introducewedge_labels which take a list of strings or a format string to be formatted with.format(absval=absval, frac=frac), or a sequence of those. See the whatsnew entry and its linked example for details.
    • Note I have usedabsval rather thanabs agreed 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.
  • Introducewedge_label_distance (default 0.6) to specify how far the above are from the pie centre.
  • Ifwedge_labels has multiple sets of labels,wedge_label_distance must be a sequence of the same length. Otherwise error.
  • Deprecate thelabeldistance parameter in two steps: First change the default toNone (which meanslabels are only used for the legend). Later remove this parameter altogether - direct to usewedge_labels instead.
  • Discourage usingautopct in favour ofwedge_labels. If passed, internally incorporate it into thewedge_label handling.

PR checklist

# 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)
Copy link
MemberAuthor

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:

# 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.

@rcomer
Copy link
MemberAuthor

rcomer commentedNov 17, 2024
edited
Loading

@rcomerrcomerforce-pushed thepie-fmt branch 2 times, most recently frome5db08a to925704eCompareNovember 17, 2024 16:42
@rcomerrcomer added this to thev3.11.0 milestoneNov 17, 2024
@timhoffm
Copy link
Member

If we are touching the existing API, should we reconsider more fundamentally?

Currently,pie() has two sets of annotations

  1. "label", which are strings and by default displayed outside next to the wedges
  2. "pct", which are percent numbers and by default displayed inside the wedges

Some observations:

  • "pct" is a too specific name, "numbers" would be better.
  • That labels are outside and pct inside is an arbitrary choice.

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 locationinner_label andouter_label, and each could be (i) an explicit list of strings (ii) a function (iii) a format-string, likely{}-based, because that allows direct percent-formatting without needing numeric action on our side. However, I belive this would be too much of a change for the current API.

Therefore, I propose to

  • keeplabels,label_distance as is
  • introduceinner_labels andinner_label_distance.inner_labels should take (i) a list (ii) a function (iii) a{} format string
  • autopct translates toinner_labels and giving both is an error. Discourageautopct in favor ofinner_labels and describe the transition (note that autopct's %-format and callable take values that are scaled by 100).
  • pctdistance translates toinner_label_distance and giving both is an error. Discouragepctdistance in favor ofinner_labels_distance and state that you should use the variable that is matching to the one used to define the labelsinner_labels=..., inner_label_distance=... andautopct=..., pctdistance=....
  • optionally, expandlabels to also take (ii) a function (iii) a{} format string

Does that sound reasonable?

@rcomer
Copy link
MemberAuthor

rcomer commentedNov 17, 2024
edited
Loading

I am not sure aboutinner_labels because there is nothing to stop you swapping the positions of these withlabels (using the distance parameters), at which point they become outer labels. We also need some way to choose what is being passed through the function or format string: existing functionality is percentages but the feature request is to support the absolute values.

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 🤔

story645 reacted with thumbs up emoji

@rcomer
Copy link
MemberAuthor

Also note thatlabels is special because it will be automatically used bylegend (and if you want them in the legend and not on the pie you setlabeldistance=None). So that's another reason not to touch that. So the other labels could be named something relative to that:extra_labels,secondary_labels....?

The original proposal in#19338 was forwedgelabels, so just throwing that in...

timhoffm reacted with eyes emoji

@rcomerrcomer added the status: needs comment/discussionneeds consensus on next step labelNov 18, 2024
@timhoffm
Copy link
Member

What I've missed in my above thought is that{} direct percent-formatting takes case of the factor 100 between relative and percent numbers, but it does not solve the issue where to pass relative (or percent) vs. absolute numbers to the formatter. That extra information that we'd need to get in somehow.

I'm trying to find a consistent way to map the option space:

  • We need potentially two labels per wedge, typically one of them is inside and one is outside, but the positions are freely adjustable.
  • We have three kinds of information: 1. Fixed arbitray labels 2. absolute numbers 3. percent (or relative numbers)
    Note that the number variants are convenience. A user could take the input values, transform them to whatever string they want and pass them as fixed label.

There are two ways for structuring:

  • Have two label mechanisms that are structurally equivalent (i.e. allow fixed values and numerics). The distinction would be the (default) position. This was my thought above.
  • Distinguish by semantics, i.e. one set of args for fixed labels, one set of args for numeric labels, ...
    Here we actually get into some complication because either we have to define the sets of args (fixed, absolute, percent) but map them to two positions; or we have to merge absolute and percent to one parameter to one numbers parameter and add a flag to determine absolute vs relative. Either way is messy.

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 beinner_labels andouter_labels (orlabels when considering compatibility) as lists of fixed texts. Everything else is convenience. Possibly, we don't even need absolute value support then. It is justpie(x, inner_labels=[f'{xi:.1f}' for xi in x]) IMHO this is even better - because more explicit - thanpie(x, absolutefmt="%.1f"). Percent formatting is only marginally more complexpie(x, inner_labels=[[f'{xi:.1%}' for xi in x/x.sum()]).

I therefore revise my above proposal to the following minimal change:

  • keeplabels,label_distance as is
  • introduceinner_labels andinner_label_distance.inner_labels should take a list only
  • autopct translates toinner_labels and giving both is an error. Describeautopct as a higher-level convenience function to get percent values into inner_labels.
  • Documentpctdistance as equivalent toinner_label_distance and giving both is an error. State that you should use the variable that is matching to the one used to define the labelsinner_labels=..., inner_label_distance=... andautopct=..., pctdistance=....

One can later decide whether both of the labels parameters should grow some formatting / function capability.

@timhoffm
Copy link
Member

timhoffm commentedNov 18, 2024
edited
Loading

I've just read your comment on legend. That's actually making it more complicated.

plt.pie(x, labels=['a', 'b', 'c'])plt.legend()

plots both the outer labelsand the legend, which is most likely not what one wants. One has to additionally passlabeldistance=None, but that prevents using the outer labels for something else (e.g. absolute numbers).

Note that outer labels are a good option if wedges become tiny. Therefore I fundamenally would want capability to put anything outside, e.g.
image

I believe right now, the only way to add a legend there is explicitly passing the artists and legend labels tolegend().

A green-field solution would be to havelabels only for the legend and explicitouter_labels andinner_labels on top. This separates concerns. However that'd be quite disruptive.

I have to rethink what we can do here.


Edit: I think we could make a clean transition path for separatinglabels intolabels (legend only) +outer_labels:

  • If as user useslabels with a not-Nonelabeldistance, they want an outer-wedge label. Add a deprecation here and state that they should useouter_labels (andouter_label_distance if needed). Add an additional note that they need to pass the same list tolabels as well if they want to keep legend entries.
  • If a user has usedlabels=..., labeldistance=None they only want and get a legend. Nothing to do for a start. One can deprecate and remove thelabeldistance parameter, when the above deprecation on forlabels generating outer texts has run out. Thenlabels will always only create legend entries and thelabeldistance parameter has no effect anymore.

The only downside is the need for users to adapt their code. But that's inevitable if we want a logical separation between legend labels and wedge labels.

This all is independent of the "absolute value" topic, but would fit together with the proposed solution introducinginner_labels there.


I am not sure aboutinner_labels because there is nothing to stop you swapping the positions of these withlabels (using the distance parameters)

I'm not concerned about this. Practically, there's no need to swap - you'll rather swap the lists passed tolabels/inner_labels. But if you do that's a very conscious and explicit decision.

namingextra_labels,secondary_labels,wedge_labels

  • wedge_labels is a bit ambiguous. While the inner labels areon the wedge, both labels are per-wedge. The outside labels in the plot above could also be regarded as "wedge_labels".
  • extra_labels,secondary_labels implies a hierarchy, which IMHO does not exist. Both places are logically equivalent. Havingextra_labels/secondary_labels withoutlabels would be a valid use case, but sounds a bit awkward. Additionally, extra/secondary does not give you an idea what they are used for / where they will appear.

@rcomer
Copy link
MemberAuthor

I am also wondering if it’s worth factoring out a publicpie_label method, to rhyme withbar_label. The existinglabels andautopct could remain as conveniences that internally call this method, but a user could also call it directly as many times as they like to get whatever combination of labels they like.

@timhoffm
Copy link
Member

pie_label method: I'm a bit sceptical. If we didn't have labelling functionality at all inpie, that might be the way to go. But as is, there's not much new functionality to be introduced via apie_label. More than two positions (inside/outside) would be a very edge case. And the fancy number formatting/percent stuff is quite optional, because you can do it out-of-the-box with little overhead I wouldn't introduce new just API for that.

What may be a good idea is a private_wedge_text() method as internal refactoring, which would simplify the label handling in pie.

@rcomer
Copy link
MemberAuthor

rcomer commentedNov 18, 2024
edited
Loading

OK let me see if I can summarise the current thinking:

  • Introduceinner_labels andouter_labels which take a list of strings or a format string to be formatted with.format(abs=abs, frac=frac).
  • Introduceinner_label_distance (default 0.6) andouter_label_distance (default 1.1) to specify how far the above are from the pie centre.
  • Deprecate usinglabels with not-Nonelabeldistance - direct to use outer labels instead.1
  • Discourage2 usingautopct in favour ofinner_labels. Passing both is an error.
  • Passingpctdistance andinner_labels is an error.
  • Passinginner_label_distance andautopct is an error.

Footnotes

  1. when this deprecation has expired, deprecate thelabeldistance parameter altogether.

  2. it might be nice to eventually deprecateautopct as having 4 parameters for labels is confusing.

@timhoffm
Copy link
Member

timhoffm commentedNov 18, 2024
edited
Loading

Good summary! This isone viable way to make the API more consistent.

I think we should offer the formatting option as it will be hard to discourage use of autopct if the replacement is less convenient.

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 unintuitivepie(x, inner_labels="%.1f").

The function approach is a bit better, because you could support two signaturesdef fmt(abs_value: float) -> str anddef fmt(abs_value: float, total: float) -> str. The first one is a simpler version for convenince, the second one would support percents aspie(values, inner_label=lambda x, total: f'{x/total:.1%'}).


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 - currentlytextprops are shared androtatelabels only applies to the outside.

Problem 1: AFAICS, we cannot retrieve the underlying data value from a wedge. Therefore no external function can create labels based on the output ofpie(). We'd likely need to create a semantic artistPieWedge or a kind ofPie container class to handle this.
Problem 2: Removing all labelling capability frompie and forcing the use of additional functions feels a bit drastic. The alternatives would be to keep some labelling capability and that's not great either: (i) Keepinglabels as the simple generic variant keeps the entanglement with legend (ii) Keeping autopct is very specific (iii) changing to a new single labels API forces a migration and makes keeping an additional text labels API only half as valuable.

If feel this is less good than the summarized approach.

Footnotes

  1. Edit: It's a bit unusual, but we might solve the absolute/relative issue with a text prefixinner_labels="percent:%.1f" could replaceautopct="%.1f" - orinner_labels="fraction:{:.1%} when using format-string notation.

@jklymak
Copy link
Member

Completely green fielding this issue a I'm pretty ignorant aboutpie - I would personally get rid of autopcnt - it's not like our users cannot calculate the data in terms of a percentage, and the pie chart would look the same. Then the formatting of the quantitative label would be straightforward.

@rcomer
Copy link
MemberAuthor

rcomer commentedNov 19, 2024
edited
Loading

I would prioritise formatting fractions over absolute values:

  • Since plotting fractions is basically the point ofpie I don’t think it’s too unintuitive that those would have the convenience in terms of labelling. The canonical example would bepie(x, inner_labels="{:.1%}"). Also sincepie already calculates the fractions in order to draw the wedges it feels a bit annoying/redundant for the user to have to pre-process the data just for the labels (even if it is a simple and cheap calculation).
  • Absolute values are slightly more straightforward to convert to the desired string. In fact if the user has ints then they may not care about formatting, so they could just pass the list of numbers as labels and theText constructor will take care of converting them to strings.

Edit: I guess youcould detect whether the format string contains "%}". If it does, give it fractions. If not give it absolute values.

@timhoffm
Copy link
Member

timhoffm commentedNov 19, 2024
edited
Loading

Edit: I guess youcould 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{.1f} and{.1%} relate to each other. We shouldn't mess with that. Then let's rather do the"percent:..." or"relative:..." or"fraction:..." prefix to the format string.

rcomer reacted with thumbs up emoji

@rcomer
Copy link
MemberAuthor

rcomer commentedNov 19, 2024
edited
Loading

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 reacted with thumbs up emoji

@timhoffm
Copy link
Member

Named placeholders would give a lot more flexibility...

Great idea. Add this format-string as acceptable input forinner_labels andouter_labels in#29152 (comment) and we have a compact and clean API.

The fundamental question is: Do we want to go through the hassle of repurposinglabels to only apply to legend. If yes, this is the reasonable way forward. If no, only doinner_labels as a replacement ofautopct. I'm 0.5 on going all the way (but slowly, i.e. start with a pending deprecation).

@rcomer
Copy link
MemberAuthor

I am in favour of makinglabels only for legend.

  • API consistency: almost everywhere that you passlabel(s) in Matplotlib, you mean you want them in the legend.
  • For the case where you want both a legend and numbers outside the wedges, usinglabels=list_of_strings, outer_labels=... is more intuitive to me (and simpler) thanlabels=list_of_strings, labeldistance=None, inner_labels=..., inner_label_distance=1.1.
story645 and timhoffm reacted with thumbs up emoji

@timhoffm
Copy link
Member

timhoffm commentedNov 20, 2024
edited
Loading

Side note on naming: I've briefly pondered whether in would make sense to bikeshedouter_labels,inner_labels to replace the "label" part; motivation: set it more clearly apart fromlabels; e.g. go forwedge_text or similar. However, we have ample precedence for*_label usages independent oflabels mapping to legend:bar_label,clabel (contour label),tick_labels,xlabel,ylabel. Thereforeouter_labels andinner_labels seem perfectly fine.

Edit: The only other alternative that may be worth considering is to make a singlewedge_labels,wedge_label_distance that accepts a single list or a tuple of lists so that one can do:

plt.pie(x, wedge_labels=['a', 'b', 'c'], wedge_label_distance=0.5)plt.pie(x, wedge_labels=(['a', 'b', 'c'], ['A', 'B', 'C']), wedge_label_distance=(0.5, 1.2))plt.pie(x, wedge_labels="{frac:.1%}")plt.pie(x, wedge_labels=("{frac:.1%}", "{abs:.2f}"), wedge_label_distance=(0.5, 1.2))

instead of

plt.pie(x, inner_labels=['a', 'b', 'c'], inner_label_distance=0.5)plt.pie(x, inner_labels=['a', 'b', 'c'], inner_label_distance=0.5, outer_labels=['A', 'B', 'C'], outer_label_distance=1.2)plt.pie(x, inner_labels="{frac:.1%}")plt.pie(x, inner_labels="{frac:.1%}", inner_label_distance=0.5, outer_labels="{abs:.2f}", outer_label_distance=1.2)
rcomer and story645 reacted with thumbs up emoji

@story645
Copy link
Member

story645 commentedNov 20, 2024
edited
Loading

The only other alternative that may be worth considering is to make a single wedge_labels, wedge_label_distance that accepts a single list or a tuple of lists

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 functionpie_label? that could then take a third parameter "fmt" that accepts a list of formatting strings/functions/Formatter Objects

@timhoffm
Copy link
Member

Though could this also be the external functionpie_label?

Not easily. See#29152 (comment) for the issues I see withpie_label function.

@rcomer
Copy link
MemberAuthor

What would be the default forwedge_label_distance?

@rcomerrcomer marked this pull request as ready for reviewOctober 19, 2025 16:02
@rcomer
Copy link
MemberAuthor

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
Copy link
Member

timhoffm commentedOct 21, 2025
edited
Loading

Sorry for letting this slip. I will do a review soon.

Looking at this with some distance, I'm undecided whetherwedge_labels for one or more labels orinner_labels /outer_labels is the better solution.

On the one hand, a single parameter is generic and efficient. On the other hand, it's getting a bit complex and almost ambiguous:wedge_labels=["spam", "eggs"] would be a single set of labels. Butwedgelabels=["{absval:d}", "{frac:.0%}"] would be two sets of labels.

@rcomer since you have worked with this in detail, what do you think? Are we trying to be too clever here?

@rcomer
Copy link
MemberAuthor

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"])
image
plt.pie([5,2],wedge_labels=["{absval:d}","{frac:.0%}"])
ValueError: Found 2 sets of wedge labels but 1 wedge label distances
plt.pie([5,2],wedge_labels=["{absval:d}","{frac:.0%}"],wedge_label_distance=[0.6,1.1])
image

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
Copy link
Member

timhoffm commentedOct 21, 2025
edited
Loading

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.

Exactly. We have the following use cases (Note: i usefractions andnames as placeholders for the labels. These could be either lists or format strings):

  1. One inner label:
ax.pie(values,wedge_labels=fractions)ax.pie(values,inner_labels=fractions)
  1. One outer label:
ax.pie(values,wedge_labels=fractions,wedge_label_distance=1.1)ax.pie(values,outer_labels=fractions)
  1. Inner and outer labels (standard distances):
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)
  1. Inner and outer labels (custom distances):
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.

Relevance(1-5)wedge_labelsinner_labels,outer_labelspie(...),pie_label(...)pie(..., wedge_labels=...),pie_label(...)
1. One inner label5⭐⭐⭐⭐⭐⭐⭐⭐⭐⭐⭐⭐⭐⭐⭐⭐⭐⭐
2. One outer label5⭐⭐⭐⭐⭐⭐⭐⭐⭐⭐⭐⭐⭐⭐⭐⭐
3. Inner and outer labels (standard distances)3⭐⭐⭐⭐⭐⭐⭐⭐⭐⭐⭐⭐⭐⭐⭐⭐
3b. Inner and outer labels (magic defaults)3⭐⭐⭐⭐⭐⭐⭐⭐⭐⭐⭐⭐⭐⭐⭐⭐
4. Inner and outer labels (custom distances)2⭐⭐⭐⭐⭐⭐⭐⭐⭐⭐⭐⭐⭐
5. More than two labels1⭐⭐-⭐⭐⭐⭐⭐⭐⭐⭐

Overall,inner_labels,outer_labels perform better on the common use cases, because they are more explicit. They are only slightly worse for custom positioning of two labels (very verbose) and cannot handle more than two labels, wich is a very edge case.

Update: Included the optionspie(...),pie_label(...) (strict separation) andpie(..., wedge_labels=...),pie_label(...) (additional single-label convenience parameter onpie().

@story645
Copy link
Member

inner_labels, outer_labels perform better on the common use cases, because they are more explicit

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
Copy link
Member

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
Copy link
Member

did you consider ax.label_wedges or some such?

See#29152 (comment).

@jklymak
Copy link
Member

It seems the only issue is making the wedges store info about their value? That doesn't seem to be a huge lift.

@timhoffm
Copy link
Member

timhoffm commentedOct 21, 2025
edited
Loading

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,pie(..., label=...) sets wedge labels and is used for legend. We want to limit it to legend. Therefore people have to move topie(..., wedge_labels=...) (orinner_labels). That step is smaller and easier to search/replace than moving topie(...); pie_label(...); which would also need moving relevant parameters with it. We could alternatively keep parameters for a single label onpie() (internally delegating topie_label()) and only advertizepie_label() for further labels, but that's also inconsistent.

@timhoffm
Copy link
Member

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.

Well, there's semantics though the defaults. Yes, you can override this by intentionally choosing other parameters. But even then,outer_label_distance=0.7 is basically saying "I move the outer label to the inside of the wedge as well", which feels ok to me.

I would not worry too much about this. There's still one canonical way to do each use case. Yes you could dopie(..., outer_labels=..., outer_label_distance=0.7) to set one inner label. But let's not count this. There are various ways to intentionally write hard-to-understand code.

@story645
Copy link
Member

There are various ways to intentionally write hard-to-understand code.

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
Copy link
Member

There's a second issue in that the API changes gets larger. currently, pie(..., label=...) sets wedge labels and is used for legend.

Fair enough. Personally, I'd live with the mild inconsistency and allow extra labels with a call topie_label() with duplicated semantics topie(..., wedge_labels={}) rather than try and come up with a complicate API that allows multiple labels (and all their parameters) from insidepie().

@timhoffm
Copy link
Member

Personally, I'd live with the mild inconsistency and allow extra labels with a call topie_label() with duplicated semantics topie(..., wedge_labels={}) rather than try and come up with a complicate API that allows multiple labels (and all their parameters) from insidepie().

I've updated my evaluationabove with these variants, and it indeed seems most usable to have a simplepie(..., wedge_labels=...) plus an additionalpie_label().

rcomer reacted with eyes emoji

@rcomer
Copy link
MemberAuthor

Just to make sure I’ve understood, if we add thepie_label method together withwedge_labels, doeswedge_labels only do one set of labels?

It did feel a little clunky addingwedge_label_distance=[0.6, 1.1] through all the tests and examples 🤔

@rcomer
Copy link
MemberAuthor

If we do go that route, I propose to split the change into two PRs:

PR1. IntroducePieWedge andax.pie_label (andpyplot.pie_label?). Thepie API is unchanged except for the new artist in the return value.

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.

story645 reacted with thumbs up emoji

@timhoffm
Copy link
Member

Just to make sure I’ve understood, if we add thepie_label method together withwedge_labels, doeswedge_labels only do one set of labels?

Yes.

A split into two PRs is reasonable. We could split also in another way: (1) Modify thepie() signature towards the target state, i.e. a singlewedge_labels parameter. (2) introduce the thePieWedge andpie_label() - but I think your way is a little less work.

rcomer reacted with thumbs up emoji

@story645
Copy link
Member

As a follow up, couldpie_label expand to support all sort of styling for the text? As a follow up issue?

@timhoffm
Copy link
Member

timhoffm commentedOct 22, 2025
edited
Loading

Yespie_label() will be allowed to go fancy.

Re: Design - I believe we need some container as the return type ofpie().

We currently have

patches, texts = pie()patches, texts, autotexts = pie(..., autpct=...)

We should end up with

pie : PieContainer = pie()

so that the above stays valid, i.e. it supports unpackingpatches, text = pie (at least for a transition phase). We also want a high-level signaturepie_label(pie: PieContainer) and notpie_label(pie: list[PieWedge]).

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
Copy link
Member

Why wouldn't you return a list ofPieWedges? Do the wedges need to know each other for some of the formatting?

The point being you could just label some of the wedges if you just pass a sublist of wedges.

rcomer reacted with thumbs up emoji

@timhoffm
Copy link
Member

Because when regarding Pie as a semantic entity, you can e.g. add apie.set_data() method. You can't do that for a list of wedges. Note: This is exactly why we introducedStepPatch forstairs() and did not use a generic PathPatch.

@jklymak
Copy link
Member

Ok that's fair enough. I guess the wedges are not independent in that if you change one you need to change the others.

timhoffm reacted with thumbs up emoji

@rcomer
Copy link
MemberAuthor

OK, but can we say the initial implementation ofPieContainer has immutable data? I am starting to worry about the size/scope of this...

@timhoffm
Copy link
Member

timhoffm commentedOct 23, 2025
edited
Loading

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 (patches, texts = ax.pie(....)).

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 thentexts : list[Text] is likely not what we want to have as a long-term API.

rcomer reacted with thumbs up emoji

@rcomer
Copy link
MemberAuthor

There seems to be a consensus for thepie_label method. I will add that in a new PR and this one will be "PR2".

timhoffm reacted with thumbs up emoji

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

Reviewers

No reviews

Assignees

No one assigned

Projects

None yet

Milestone

v3.12.0

Development

Successfully merging this pull request may close these issues.

Allow option to display absolute values for pie chart

4 participants

@rcomer@timhoffm@jklymak@story645

[8]ページ先頭

©2009-2025 Movatter.jp