GeezerVison

• Should PieContainer inherit from Container as I have currently done?

Container falls short for our needs here. It is basically a tuple of artists with remove() 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, only BarContainer really fits in. Already ErrorbarContainer and StemContainer 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 general ArtistCollection or CompoundArtist.

On a side-note we want this to not only be pure collections, but be a semantic entity, like StepPatch, 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 to grouped_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 the pie_label functionality should be covered by existing pie tests that use the labels and/or autopct parameters. Is is better to duplicate this coverage?

If writing from scratch, I would mostly test the functionality on pie_label() directly and only ensure basic propagation from pie() to pie_label(). Given there are already many tests on pie() that is good enough for a start. I would not duplicate but rather move coverage out of pie() tests and into pie_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 own remove method.

For complete backwards compatibility, we need "texts" to be returned as an empty list when labeldistance is None. 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 using pandas pie, they will not get the PieContainer returned but only the Axes. Should we add the PieContainer to the Axes.containers list to support this case?

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. Artist.set_label calls Artist.pchanged, and I am wondering if that is the reason Container 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 multiple legend entries and is basically the analogue to PieContainer.

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 deprecate labeldistance not None and discourage autopct 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 of wedge_labels through **kwargs and then add its automatic ones using the pie_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 a get_label method into the Axes.containers list, you get an error in legend.

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 multiple legend entries and is basically the analogue to PieContainer.

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 for Pie, because you don't ever need a single legend entry for the set of all wedges, so Pie 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!