DataFrameGroupby.boxplot fails when subplots=False

[charlesdong1991]

• closes DataFrameGroupBy.boxplot with subplots=False fails when using column param #16748
• tests added / passed
• passes black pandas
• passes git diff upstream/master -u -- "*.py" | flake8 --diff
• whatsnew entry

[charlesdong1991]

is there a flaky test? 🤔 a lot other PRs passed tests though

[charlesdong1991]

would appreciate a lot if someone could give some hint of why the failure happens at test_converter.py.

[charlesdong1991]

Would appreciate a lot if you could give any hint on how test_converter could fail the tests? @TomAugspurger i tried to set it to xfail to avoid failures

[TomAugspurger]

Just to make sure I understand, is there any API change here for an end-user? Is this code called indirectly via DataFrame.boxplot? I want to make sure there are no changes for that, when the user is providing a MultiIndexed dataframe and columns.

[charlesdong1991]

You are absolutely right, this indeed caused a change on MultiIndexed dataframe and columns when using DataFrame.boxplot. But I test again and somehow feel it might be good change to end users. Since right now, for MultiIndexed dataframe, if you have a dataset like:

WHen you do `df.boxplot(column='two'), then an error will be raised:

But with the new change, this can be plotted, and I feel this change is more useful and users might like ti.

In the meanwhile, due to this, i found out actually DataFrame.boxplot with multiindex Dataframe are not tested at all at pandas, otherwise, this should raise an error. So I also add another test for this case, the test is exactly the same as I showed in the picture below.

Thanks again for your careful and thorough thought on the potential consequences, I am looking forward to further reviews. @TomAugspurger

[TomAugspurger]

But with the new change, this can be plotted, and I feel this change is more useful and users might like ti.

The alternative would be for the user to provide something like columns=pd.IndexSlice[:, 'two'], correct?

Just to make sure, there's no change to the case where the user-provided columns is from the outer level like bar?

My main concern is that by automatically doing this, we'd introduce some kind of ambiguity about what columns means.

[charlesdong1991]

Sorry for my late response, was out sick for a couple days.
I read your comments again and think I fully get your point. Indeed, this will also change the current behavior from the outer level like bar. Right now, if doing df.boxplot(columns='bar'), will get:

You mean we should keep this behavior, right? @TomAugspurger

But i might have to say behaviour of df.boxplot(columns='two') will change in this case. Before the change of this PR, this would raise a KeyError of ['two'] not in index. And now it plots columns containing two because of this pd.IndexSlice.

And I also have a second thought, and think we might provide another argument for boxplot, which is something like include (could be changed for something else) for multiindex columns, if True, then users could define more ambiguous columns to get their columns plotted, so columns='bar' will plot all columns as long as 'bar' is in any multi-level index. And for normal Index, just set to False in the function no matter what, since this won't affect normal index at all.

[jreback]

@charlesdong1991 can you merge master; can you summarize the issues here that were raised?

[jreback]

can you merge master

[charlesdong1991]

thanks a lot for picking it up @WillAyd Yeah, I am active, so is this PR! xD
I think there is a design issue with this PR, and now the priority is regression and new issues posted for 1.0

I will get back to your comments later next week, since I also came across an issue which requires a feature for plotting based on the Index level, might be relevant to this PR, I will try to get that feature done and then back to address and try to push it through

[WillAyd]

Can you move the whatsnew?

[simonjayhawkins]

@charlesdong1991 what's the status here?

[charlesdong1991]

yeah, i will give this PR another try later this week @simonjayhawkins

[jreback]

whats the status here?

[jbrockmendel]

@charlesdong1991 gentle ping can you rebase

[github-actions]

This pull request is stale because it has been open for thirty days with no activity. Please update or respond to this comment if you're still interested in working on this.

[charlesdong1991]

hey @jreback @WillAyd @simonjayhawkins @TomAugspurger

sorry for very late update here. I just took a look at the original issue and my past commits together with the relevant codebase, I feel it was quite inefficient and confusing. So here, I flipped most of the changes and I think now the new solution looks much better.

So the issue was with a df like below, it will fail doing df.groupby.boxplot(subplots=False, column='v'):

df = pd.DataFrame({'cat':np.random.choice(list('abcde'), 100), 
                    'v':np.random.rand(100), 
                    'v1':np.random.rand(100)})
df.groupby('cat').boxplot(subplots=False, column='v')

and it is because the data for plotting after df.groupby('cat') has changed to MI, so v no longer exists in the transformed data.

Therefore, the new solution for this is quite simple, I couple the keys of groupby values (in this case, [a, b, c, d, e]) with the column value sellers assign to column argument (in this case, v), so we have [(a, v), (b,v), (c, v), (d, v), (e, v)] and assign them to boxplot function, and then boxplot function will look for subset based on this new column values, instead of v in the original df which is used by sellers.

How does it sound now? Feedbacks and reviews are very welcomed!

Regarding the error, It seems irrelevant to this PR.
EDIT:
CI failure is gone now after commiting one more time

[jreback]

@charlesdong1991 this looks good, can you post an updated plot (and your tests look nice); merge master and ping on green-ish..

[charlesdong1991]

sure, these are plots with and without specifying column, I also update in the PR description on top.

[jreback]

@charlesdong1991 can you merge master

[charlesdong1991]

small ping here @jreback sorry for the late and long response

[jreback]

thanks @charlesdong1991 very nice!