ENH: Allow column grouping in DataFrame.plot

[NicolasHug]

• closes ENH: Group some columns in subplots with DataFrame.plot #29688
• tests added / passed
• passes black pandas
• passes git diff upstream/master -u -- "*.py" | flake8 --diff
• whatsnew entry

This is an attempt at #29688 i.e. group some columns on the same ax when calling df.plot

See this gist for illustration of different supported plots

Thanks!

[WillAyd]

@NicolasHug is this still active? Looks like a merge conflict and CI failure need to be addressed

[NicolasHug]

I'm still happy to work on this. I just haven't received initial feedback yet (and don't want to push either, I understand this isn't necessarily a high priority)

Before working more on this I'd appreciate some feedback to make sure this is going the in right direction, and that this is actually a desired feature.
Feel free to close otherwise, or LMK if you need to see more to decide :)

[WillAyd]

Understood. I doubt anyone has reviewed since CI has been red, so definitely want to get that green to start

[NicolasHug]

Will do

[WillAyd]

@charlesdong1991 any thoughts on this?

[charlesdong1991]

I've noticed this feature request a while ago and personally really like this feature and find it quite potentially useful ^^ @WillAyd but I think the behaviour of this needs to be well defined.

Many thanks for the PR, it's very nice! @NicolasHug some minor comments:

1. As mentioned in review, the input for subplots needs to be carefully defined, thus only checking if it is Iterable or bool might not be sufficient.
2. And it would be very nice if you could add some detailed docstring for subplots parameter because this is a behaviour change, i think somewhere in PlotAccessor in _core.py.
3. And some more tests would be appreciated (see comments below)
4. Nicer to have a docstring for _col_idx_to_axis_idx although i think it is quite straightforward
5. Nicer to have a whatsnew note in 1.1.0 rst !

[NicolasHug]

Thanks a lot for the review @charlesdong1991 , I addressed your comments and added a bunch of validation / tests.

It looks like the code works for most kinds of plots. You can see a few illustrations in this gist.

Looks like the subplot titles could be improved in some cases, I'll see what I can do. But otherwise I think this is ready for another round ;)

[NicolasHug]

Regarding the docstring, I'm a bit confused because it seems that subplots have been removed from most docstrings? i.e. git grep "subplots :" only gives one hit, for boxplot_frame_groupby(). I can't find it for PlotAccessor.

[charlesdong1991]

very nice! some minor comments

Could you also please make some docs for subplots? I think you could add it in PlotAcessor, there are some missing docs for parameters, so very nice this one is found out! ^^

[charlesdong1991]

looking at your impressive examples, maybe we should exclude pie chart here? doesnt seem to fit as good as other charts here? any thoughts on it?

[NicolasHug]

I agree it doesn't look good, but it seems to be consistent (i.e. equally bad) as when subplots=True. I updated the notebook at the end where you can see how it looks. So maybe it can be kept? No strong opinion!

[charlesdong1991]

i have no strong opinion either, my only concern is for pie chart, if they share the same category, they will have the same color in the pie, but different sizes depending on the corresponding values.

but no strong opinion

[charlesdong1991]

should only not isinstance(group, Iterable) be enough here? isinstance(group, str) also falls into this, right?

[NicolasHug]

The thing is, we want group to be an iterable but we don't want group to be a str, which is also iterable.

Only using if any(not isinstance(group, Iterable) would allow strings and we don't want that

[ivanovmg]

Looks good to me.
Pending CI checks.

[ivanovmg]

I suppose, that once you added return type, mypy started complaining.
Inside the loop you return only in case if col_idx in group, this is the reason for the mypy error.
I suggest that you modify it:

for group_idx, group in enumerate(self.subplots):
    if col_idx in group:
        return group_idx
raise KeyError(f"Failed to find {col_idx} in {self.subplots}")

Or something like this.

[jreback]

@NicolasHug was looking good. if you want to continue, pls merge master and can relook.

[jreback]

sorry for the delay here

@NicolasHug was looking good. if you want to continue, pls merge master and can relook.

[NicolasHug]

PR is up to date with main @jreback

[jreback]

@NicolasHug can you merge master and we'll see if can get in

[mroeschke]

Thanks for sticking with this @NicolasHug. I merged master and did a small refactor to validate the subplot groups in one pass.

I think this looks pretty good to merge. I believe it requires @jreback approval to move forward.

[jreback]

thanks @NicolasHug very nice!