BUG/REF: Refactor BaseWindowIndex so groupby.apply has a consistent index output #39765
Conversation
mroeschke
commented
Feb 12, 2021
mroeschke
commented
- closes BUG: Resulting index from groupby.apply is different depending on whether a RollingGroupby object is created #39732
- tests added / passed
- Ensure all linting tests pass, see here for how to run them
- whatsnew entry
mroeschke
added
Bug
Refactor
pandas/core/window/rolling.py
Outdated
| self._groupby = groupby | ||
| self._groupby.mutated = True | ||
| self._groupby.grouper.mutated = True | ||
| _attributes = [ |
There was a problem hiding this comment.
rather than doing this. can you create a private classmethod, instantiate and mutate the returned object? e.g.
@classmethod _create_from_grouping(cls,, grouping_indices, keys, codes......):
obj = cls(.......)
obj._grouping_indices = grouping_indcies
.....
return obj
There was a problem hiding this comment.
This attribute (_attributes) is currently necessary to the window classes for _gotitem where a shallow copy is performed, so I would still need to keep this reference
pandas/core/window/rolling.py
Outdated
| _grouping_levels=None, | ||
| **kwargs, | ||
| ): | ||
| self._grouping_indices = _grouping_indices or {} |
There was a problem hiding this comment.
see above but maybe worthile having a dataclass holding all of these things (so 1 object) e.g.
class Groupings:
indices: .....
There was a problem hiding this comment.
you might also just be able to pass the Grouper object itself.
There was a problem hiding this comment.
Yeah good point. I'll just pass the grouper object
pandas/core/window/rolling.py
Outdated
| _grouping_levels=None, | ||
| **kwargs, | ||
| ): | ||
| self._grouping_indices = _grouping_indices or {} |
pandas/core/window/rolling.py
Outdated
| _grouping_levels=None, | ||
| **kwargs, | ||
| ): | ||
| self._grouping_indices = _grouping_indices or {} |
There was a problem hiding this comment.
you might also just be able to pass the Grouper object itself.
pandas/tests/window/test_groupby.py
Outdated
| @@ -101,6 +107,9 @@ def test_rolling_quantile(self, interpolation): | |||
| g = self.frame.groupby("A") | |||
| r = g.rolling(window=4) | |||
| result = r.quantile(0.4, interpolation=interpolation) | |||
| # GH 39732 | |||
| g.mutated = True | |||
There was a problem hiding this comment.
why are you setting all of these? what is the purpose?
There was a problem hiding this comment.
To get the resulting index of g.apply(lambda x: x.rolling(...) to match self.frame..groupby(...).rolling(...), the mutated attributes on g need to be set to True
It directly why #39732 was a bug
There was a problem hiding this comment.
I will change the expected indexes instead of setting this attribute
|
lgtm. can merge on green. |
|
CI failures were related to the timedelta issue that has already been fixed |
