• ShouldPieContainer inherit fromContainer as I have currently done?

Container falls short for our needs here. It is basically a tuple of artists withremove() propagation and a CallbackRegistry (which AFAICS we nowhere use). That simple tuple approach does not suit the more general case that we have not just one type sequence of Artists. In fact, onlyBarContainer really fits in. AlreadyErrorbarContainer andStemContainer have to work around the limitation using tuples of tuples, which is not a good API.

I think we need to come up with a more generic concept. For a start, I'd say make your own type (i.e. copy the remove method) and define whatever minimal API is reasonable. If we know what kind of API we want to have based on this example it's easier to design a more generalArtistCollection orCompoundArtist.

On a side-note we want this to not only be pure collections, but be a semantic entity, likeStepPatch, i.e. there may be (not necessarily from the start) semantic functions, that non-trivially manipulate multiple properties of the artists; e.g. setting the pie values would need to recompute positions of all wedges and texts and update the text content.

Edit: The case is similar togrouped_bar(), where we currently also only have a provisional class with functionality and minimal guarantees.

• So far I have added very few new tests, since most of thepie_label functionality should be covered by existingpie tests that use thelabels and/orautopct parameters. Is is better to duplicate this coverage?

If writing from scratch, I would mostly test the functionality onpie_label() directly and only ensure basic propagation frompie() topie_label(). Given there are already many tests onpie() that is good enough for a start. I would not duplicate but rather move coverage out ofpie() tests and intopie_label() tests. But that is a nice-to-have. If done, I'd rather make this a separate cleanup PR.

OK,PieContainer is now its own class with its ownremove method.

For complete backwards compatibility, we need "texts" to be returned as an empty list whenlabeldistance isNone. I think both my current and previous implementations broke that.

In draft for now while I think about how to address that. Suggestions of course welcome!

Edit: this is now addressed by the next push.

Apologies - just found some doc changes that I had missed before. None of the examples now unpack or index the return value.

Thanks for all your reviewing work@timhoffm. Your latest suggestions seem good to me.

It just occurred to me that if someone is usingpandas pie, they will not get thePieContainer returned but only theAxes. Should we add thePieContainer to theAxes.containers list to support this case?

To be in the containers list we need at least aget_label method.Container takes theget_label andset_label methods fromArtist.Artist.set_label callsArtist.pchanged, and I am wondering if that is the reasonContainer has the unused callback machinery.

To be in the containers list we need at least a get_label method. Container takes the get_label and set_label methods from Artist

Does it make sense to have a container interface/ABC to spell out the membership requirements for the containers list?

We need to define what exactly a container is. My understanding from the label machinery is that it is an artist that is realized through multiple base artists, but rather as an implementation detail. ErrorbarContainer uses multiple Line2D, but is logically still a single entity that shows up as one legend entry. I haven’t checked, but suspect that tracking containers explicitly in Axes may be done so that we can identify the artists to label (but it could also be that this was done purely to have a place to attach the involved artists to the figure, since historically we tracked different types of artists like lines, patches, etc. in their own lists and we certainly want to mix the lines of BarContainer with regular standalone lines).

In contrast, Pie is a rather a concerted logical aggregation of multiple artists. A single label and legend entry does not make sense. I therefore believe that Pie is not a container as far as current container design goes.

How to move forward?

I wouldn’t jump extra hoops to support advanced plotting through the pandas plot API. It it’s a high-level API to ease plotting from dataframes, but you can always extract the column data and feed it to matplotlib if needed.

we should specify the concept behind the type of aggregation we do with Pie more clearly. And we need to define how that is represented in internal structures. Is it one artist that can provide multiple legend entries? Or is it rather a grouping where the individual parts are tracked as separate entities in the Axes? Currently, and historically, wedges are added as individual patches. As long as we do this, we can live without adding Pie to the Axes as a minimal starting point. Agreed it would be nice to be able to retrieve it back, but it’s not that we are losing functionality. It’s just a yet to be implemented feature which requires additional design and refactoring, and I propose to rather do this as a follow up.

A single label and legend entry does not make sense. I therefore believe that Pie is not a container as far as current container design goes

BarContainer also supports multiplelegend entries and is basically the analogue toPieContainer.

OK, I am happy to just drop the latest commit here.

I was thinking that we need to support adding (at least) two sets of labels - once we deprecatelabeldistance not None and discourageautopct in the next PR that means at least one set has to be added after the pie is created. However, I was forgetting that pandas will add one of those sets automatically based on what is in the data frame. So pandas could pass on a single set ofwedge_labels through**kwargs and then add its automatic ones using thepie_label method.

I haven’t checked, but suspect that tracking containers explicitly in Axes may be done so that we can identify the artists to label

I can confirm that if you put something without aget_label method into theAxes.containers list, you get an error inlegend.

OK, PR is now back to what@timhoffm already approved 😇

A single label and legend entry does not make sense. I therefore believe that Pie is not a container as far as current container design goes

BarContainer also supports multiplelegend entries and is basically the analogue toPieContainer.

BarContainer dues not reasonably handle labels either. There's some low-level hack in bar() to make this work.

BarContainer dues not reasonably handle labels either. There's some low-level hack in bar() to make this work.

Looks like it's doing an either use labels on individual artists or container level label. I think it's maybe a questionable API decision to make them mutually exclusive, but I don't think it's a hack.

I think it's maybe a questionable API decision to make them mutually exclusive, but I don't think it's a hack.

Whatever you want to name it. The effect is that responsibility of label handling is distributed between the individual bars and the container. There is no easy way to change between the two approaches afterwards.

But anyway, this is of no concern forPie, because you don't ever need a single legend entry for the set of all wedges, soPie itself does not need the label capability.

Have I missed something I still need to do for this one?

Thanks@rcomer for the new labeling capabilities!