Fixed Issue Preventing Agg on RollingGroupBy Objects #21323
Conversation
pandas/core/base.py
Outdated
| @@ -684,8 +684,14 @@ def _gotitem(self, key, ndim, subset=None): | |||
| # with the same groupby | |||
| kwargs = dict([(attr, getattr(self, attr)) | |||
| for attr in self._attributes]) | |||
|
|
|||
| if self._groupby.ndim == 1 and self._groupby.obj.name == key: | |||
There was a problem hiding this comment.
I hate checks like this :>
does
groupby = self._groupby[key] not just work here?
There was a problem hiding this comment.
That’s what failed in the original code. Could alternately try and catch the error
pandas/tests/test_window.py
Outdated
| @@ -520,6 +520,39 @@ def test_iter_raises(self, klass): | |||
| with pytest.raises(NotImplementedError): | |||
| iter(obj.rolling(2)) | |||
|
|
|||
| def test_agg(self): | |||
There was a problem hiding this comment.
can be a bit more descriptive here on the test name / expl
pandas/tests/test_window.py
Outdated
| ('low', 'mean'), ('low', 'max'), ('high', 'mean'), | ||
| ('high', 'min')]) | ||
| expected = pd.DataFrame([ | ||
| [np.nan, np.nan, np.nan, np.nan], |
There was a problem hiding this comment.
maye put in TestApi. this should test rolling & expanding (atm we don't have a nice modular structure to do this :<)
Codecov Report
@@ Coverage Diff @@
## master #21323 +/- ##
==========================================
+ Coverage 92.18% 92.19% +<.01%
==========================================
Files 169 169
Lines 50819 50823 +4
==========================================
+ Hits 46850 46855 +5
+ Misses 3969 3968 -1
Continue to review full report at Codecov.
|
pandas/core/base.py
Outdated
|
|
||
| try: | ||
| groupby = self._groupby[key] # get slice of frame | ||
| except Exception: |
There was a problem hiding this comment.
Ideally would catch something more descriptive than an Exception, but that is all that gets raised:
Line 249 in 67e6e6f
Maybe an AttributeError would be more appropriate?
There was a problem hiding this comment.
yeah see if u can catch something more specific bere
|
Test errors have to do with the column sorting of the returned frame. Do we make any guarantees about how that is supposed to be ordered across versions? |
|
i would use an ordered dict for tests (so no guarantees on actual ordering) |
| pytest.raises(Exception, grouped['C'].__getitem__, 'D') | ||
|
|
||
| def test_as_index_series_column_slice_raises(df): | ||
| grouped = df.groupby('A', as_index=False) |
There was a problem hiding this comment.
can you add the issue number here
pandas/core/base.py
Outdated
| @@ -684,8 +684,14 @@ def _gotitem(self, key, ndim, subset=None): | |||
| # with the same groupby | |||
| kwargs = dict([(attr, getattr(self, attr)) | |||
| for attr in self._attributes]) | |||
|
|
|||
There was a problem hiding this comment.
minor, but can you put the comments before the try (and consolidate them into 1)
|
@WillAyd can you rebase |
|
Hello @WillAyd! Thanks for updating the PR.
Comment last updated on September 25, 2018 at 19:09 Hours UTC |
|
@jreback all green |
|
thanks @WillAyd |
git diff upstream/master -u -- "*.py" | flake8 --diffAFAICT the fact that RollingGroupBy could not use
aggwith a list of functions is simply due to the fact that the GroupByMixing it inherits from could not handle the reduction in dimensions that occurs via the normal aggregation functions.