BUG: Rolling groupby should not maintain the by column in the resulting DataFrame

[MarcoGorelli]

• closes Rolling groupby should not maintain the by column in the resulting DataFrame #32262
• tests added / passed
• passes black pandas
• passes git diff upstream/master -u -- "*.py" | flake8 --diff
• whatsnew entry

Does this seem like the correct idea? If so, I'll expand on tests / write whatsnew / think about a cleaner patch

EDIT

lots of unwanted changes here, just seeing what tests fail

EDIT

need to check the types make sense and see what's the proper way to write

kwargs["exclusions"] = self.exclusions

[WillAyd]

Thanks for taking a look! @mroeschke might also have thoughts

[MarcoGorelli]

@WillAyd thanks for your review - I've written it a different way (so no side-effects this time)

[WillAyd]

Thanks - this looks like a better approach. Just wondering if there's something even cleaner out there (a lot of the groupby stuff is a mess as is...)

[jreback]

see my comments. The OP is only partially correct. If you do .rolling.groupby (or any of the variants) and .apply then you will get back the grouping column, just like groupby.apply does; so this should not change.

e.g.

In [1]: df = pd.DataFrame({'A': [1, 1, 2], 'B': [1, 2, 3]})                                                                                                                                                                                                     

In [2]: df.groupby('A').sum()                                                                                                                                                                                                                                   
Out[2]: 
   B
A   
1  3
2  3

In [3]: df.groupby('A').apply(lambda x: x.sum())                                                                                                                                                                                                                
Out[3]: 
   A  B
A      
1  2  3
2  2  3

The issue here is that .rolling.groupby happens to use .apply as an implementation detail, so that's the behavior you get. I am open to patching this only there.

[MarcoGorelli]

Thanks @jreback - that probably simplifies the patch then.

What about if we use .resample? e.g.

>>> df
           id  score
date                
2010-01-01  A      2
2010-01-02  A      3
2010-01-05  A      8
2010-01-10  A      7
2010-01-13  A      3
2010-01-01  B      5
2010-01-03  B      2
2010-01-04  B      1
2010-01-11  B      7
2010-01-14  B      3

>>> df.groupby("id").resample("D").asfreq()
                id  score
id date                  
A  2010-01-01    A    2.0
   2010-01-02    A    3.0
   2010-01-03  NaN    NaN
   2010-01-04  NaN    NaN
   2010-01-05    A    8.0
   2010-01-06  NaN    NaN
   2010-01-07  NaN    NaN
   2010-01-08  NaN    NaN
   2010-01-09  NaN    NaN
   2010-01-10    A    7.0
   2010-01-11  NaN    NaN
   2010-01-12  NaN    NaN
   2010-01-13    A    3.0
B  2010-01-01    B    5.0
   2010-01-02  NaN    NaN
   2010-01-03    B    2.0
   2010-01-04    B    1.0
   2010-01-05  NaN    NaN
   2010-01-06  NaN    NaN
   2010-01-07  NaN    NaN
   2010-01-08  NaN    NaN
   2010-01-09  NaN    NaN
   2010-01-10  NaN    NaN
   2010-01-11    B    7.0
   2010-01-12  NaN    NaN
   2010-01-13  NaN    NaN
   2010-01-14    B    3.0

This is the output that's currently being tested for in pandas/tests/resample/test_resampler_grouper.py - this one's correct as it is, then?

[MarcoGorelli]

@jreback have pushed a new fix. Have had to update some tests which check output against groupby.apply

[jreback]

this will affect groupby.rolling and groupby.resample but NOT groupby.apply

[jreback]

WAT?

[jreback]

WAT? i would much rather properly copy in the super class. really avoid if/then cases unless absolutely necessary.

[jreback]

IOW we can always just copy exclusions

[MarcoGorelli]

@jreback sure, I can remove the if-then cases here, but then I need to potentially remove elements from self.exclusions in

def _obj_with_exclusions(self)

, otherwise this could throw an error if there are elements of self.exclusions that are no longer part of self.obj.columns:

return self.obj.drop(self.exclusions, axis=1)

[MarcoGorelli]

Anyway, that's what I've done in the latest commit

[jreback]

are you sure this line is actually needed? this is very very odd to do

[jreback]

copying the exclusions should be enough. you maybe be able to move this shallow copy to pandas/core/groupby/groupby.py which is better than here.

[MarcoGorelli]

The reason for doing this was that RollingGroupby._groupby._python_apply_general operates on RollingGroupby._groupby._selected_obj.
And RollingGroupby._groupby._selected_obj returns RollingGroupby._groupby.obj, possibly sliced by RollingGroupby._groupby._selection

[MarcoGorelli]

you maybe be able to move this shallow copy to pandas/core/groupby/groupby.py

As in, define _GroupBy._shallow_copy? I don't understand how that would work: when we call Rolling.sum, we go to _Rolling_and_Expanding.sum and then WindowGroupByMixin._apply. Inside there, we have

        def f(x, name=name, *args):
            x = self._shallow_copy(x)

            if isinstance(name, str):
                return getattr(x, name)(*args, **kwargs)

            return x.apply(name, *args, **kwargs)

Here, self is WindowGroupByMixin. In the case when we get here from Rolling.sum, then self._shallow_copy will take us directly to ShallowMixin._shallow_copy. Therefore, even if we define _GroupBy._shallow_copy, the execution won't get there during this operation.

EDIT

Anyway, am trying a slightly different approach, will ping if/when green and once I've thought over why corr and cov currently use ._selected_obj instead of ._obj_with_exclusions

[MarcoGorelli]

@jreback thanks for your review. I've pushed a new attempt at a solution.

I patches obj before we get to _GroupBy.apply, and only for Rolling so that previous behaviour in DataFrame.groupby.apply.

[jreback]

this is not going well. there is mutation all over the place. This needs a good clean solution to get merged.

[jreback]

Add an expl here, a couple of sentences; this the issue number.

[jreback]

huh? why are you doing all of this

[jreback]

I really don't like this

[MarcoGorelli]

Yes, I see what you mean - to be honest I can't think of a clean solution, and have spent a while on it already. I'll close this for now so someone else can take it up