-
-
Notifications
You must be signed in to change notification settings - Fork 18.4k
BUG: loffset has no effect when passing in a numpy.timedelta64 #22482
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
pandas/core/resample.py
Outdated
@@ -365,8 +365,9 @@ def _apply_loffset(self, result): | |||
the result of resample | |||
""" | |||
|
|||
loffset_types = (DateOffset, timedelta, np.timedelta64) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
np.timedelta64
is not the instance of timedelta
restarted the failed travis worker. |
Could you add a release not in 0.24.0.txt? |
Codecov Report
@@ Coverage Diff @@
## master #22482 +/- ##
==========================================
+ Coverage 92.04% 92.04% +<.01%
==========================================
Files 169 169
Lines 50787 50787
==========================================
+ Hits 46745 46746 +1
+ Misses 4042 4041 -1
Continue to review full report at Codecov.
|
f118371
to
19d1394
Compare
@pytest.mark.parametrize('loffset', [timedelta(minutes=1), | ||
'1min', Minute(1), | ||
np.timedelta64(1, 'm')]) | ||
def test_resample_loffset(self, loffset): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we put a comment referencing the issue next to the relevant input value to loffset
?
7e3b383
to
10a28f5
Compare
doc/source/whatsnew/v0.24.0.txt
Outdated
@@ -703,6 +703,7 @@ Groupby/Resample/Rolling | |||
- Multiple bugs in :func:`pandas.core.Rolling.min` with ``closed='left'` and a | |||
datetime-like index leading to incorrect results and also segfault. (:issue:`21704`) | |||
- Bug in :meth:`Resampler.apply` when passing postiional arguments to applied func (:issue:`14615`). | |||
- Bug in :meth:`Series.resample` when passing numpy.timedelta64 to `loffset` kwarg (:issue:`7687`). |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
double backticks around numpy.timedelta64
pandas/core/resample.py
Outdated
needs_offset = ( | ||
isinstance(self.loffset, (DateOffset, timedelta)) and |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
go ahead and inlinethese (add an overall comment if you would)
cc @jbrockmendel we should define scalar function helpers for this as we repeat lots of this in ops....
@@ -1173,27 +1173,20 @@ def test_resample_frame_basic(self): | |||
df.resample('M', kind='period').mean() | |||
df.resample('W-WED', kind='period').mean() | |||
|
|||
def test_resample_loffset(self): | |||
@pytest.mark.parametrize('loffset', [timedelta(minutes=1), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
if you want to use hypothesis would be great here :> (can be followup, not sure how easy)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There are no many examples in pandas codebase how to use hypothesis. Furthermore, I know only 4 cases which need to test: timedelta(minutes=1), '1min', Minute(1), np.timedelta64(1, 'm')
. Did I miss something?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ping @jreback
10a28f5
to
3e2ef06
Compare
3e2ef06
to
4db1e81
Compare
thanks @discort |
git diff upstream/master -u -- "*.py" | flake8 --diff