BUG: loffset not applied when using resample with agg() (GH13218) #13861
Conversation
| df = DataFrame({'A': vals, 'B': vals}, index=rng) | ||
|
|
||
| result = df.resample('10S', loffset='2h').agg({'A': 'count', | ||
| 'B': 'mean'}) |
There was a problem hiding this comment.
can u test for a series aggregation as well
Current coverage is 85.29% (diff: 100%)@@ master #13861 diff @@
==========================================
Files 139 139
Lines 50164 50168 +4
Methods 0 0
Messages 0 0
Branches 0 0
==========================================
+ Hits 42788 42791 +3
- Misses 7376 7377 +1
Partials 0 0
|
| @@ -310,6 +310,9 @@ def aggregate(self, arg, *args, **kwargs): | |||
| *args, | |||
| **kwargs) | |||
|
|
|||
| if isinstance(arg, dict): | |||
There was a problem hiding this comment.
maybe we should do this check inside _apply_loffset
|
in any event, pls add the test for timedelta index, but assert it fails (and put a TODO for the xref issue #7687 ) |
|
@jreback I'm not quite sure what you mean by put a TODO, could you please clarify? Anyways, doing the tests was kind of tricky, so please lmk if I went in the right direction. |
|
what I mean is can leave the timedelta fixes for another PR to fix |
|
@jreback for future reference, when I update my PRs should I @ someone with write access to let them know? Or should I just wait, and check on them intermittently, rebasing when they get out of date? thanks |
| result = df.resample('2D', loffset='2H').count() | ||
|
|
||
| # GH 13022, 7687 resample w/ TimedeltaIndex results in incorrect index | ||
| if isinstance(df.index, TimedeltaIndex): |
There was a problem hiding this comment.
no - put in the separate test classes as I indicated
|
you can ping me if u want me to look I added some comments |
|
@jreback split the tests into separate test classes |
| @@ -368,7 +370,7 @@ def _gotitem(self, key, ndim, subset=None): | |||
| return grouped | |||
|
|
|||
| def _groupby_and_aggregate(self, how, grouper=None, *args, **kwargs): | |||
| """ revaluate the obj with a groupby aggregation """ | |||
| """ reevaluate the obj with a groupby aggregation """ | |||
| the result of resample | ||
| arg : string or dict, optional | ||
| only passed in when agg() is used w/ resample | ||
| if string is passed to agg, loffset is already applied |
There was a problem hiding this comment.
this is VERY confusing. pls explain.
|
@jreback I think I explained the use of I also didn't realize that |
wcwagner
force-pushed
the
bug/13218
branch
2 times, most recently
from
76ebcf6 to
e2c3549
Compare
|
@jreback when you have time can you take a look at this, thanks |
| ---------- | ||
| result : Series or DataFrame | ||
| the result of resample | ||
| arg : string, list, or dict, optional |
There was a problem hiding this comment.
this needs to be refactored to not do this. It is unlike anything else in pandas to pass an argument to determine state.
|
@jreback I've tried, but can't think of any more alternative solutions. I went back to what I had originally, with the logic in the if this is insufficient, please lmk if you want me to close the PR |
|
well, then reopen if you have a better way. |
|
@jreback I force pushed but I don't have the option to reopen. I don't wanna create a dupe, so is there any way you can reopen this? |
git diff upstream/master | flake8 --diff