Skip to content

BUG: Fix resample with np.timedelta64 loffset has no effect #18708

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.

Sign up for GitHub

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

Jump to bottom
Closed
wants to merge 2 commits into from
Closed

BUG: Fix resample with np.timedelta64 loffset has no effect #18708

wants to merge 2 commits into from

Conversation

@Licht-T
Copy link
Contributor

@Licht-T Licht-T commented Dec 10, 2017

@pep8speaks
Copy link

pep8speaks commented Dec 10, 2017 •
edited
Loading

Hello @Licht-T! Thanks for updating the PR.

Cheers ! There are no PEP8 issues in this Pull Request. 🍻

Comment last updated on December 10, 2017 at 02:51 Hours UTC
@Licht-T Licht-T force-pushed the fix-numpy-timedelta64-loffset-resample branch from 3f2ccbc to 77cba2b Compare December 10, 2017 02:51
jreback
jreback requested changes Dec 10, 2017
View reviewed changes
pandas/core/resample.py

if isinstance(loffset, compat.string_types):
loffset = to_offset(loffset)
elif isinstance(loffset, np.timedelta64):
Copy link
Contributor

@jreback jreback Dec 10, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

much better to do the conversion up front

Timedelta already handles these conversions (timedelta and np.timedelta)

offsets should continue to use to_offset
as those are converted to absolute days for Timedelta
pandas/tests/test_resample.py
loffset=timedelta(minutes=1)).mean()
# GH7687
result1 = s.resample('5min', closed='right', label='right',
loffset=np.timedelta64(1, 'm')).mean()
Copy link
Contributor

@jreback jreback Dec 10, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

parameterize
@codecov
Copy link

codecov bot commented Dec 10, 2017 •
edited
Loading

Codecov Report

Merging #18708 into master will decrease coverage by 0.04%.
The diff coverage is 100%.

Impacted file tree graph

@@ Coverage Diff @@ ## master #18708 +/- ## ========================================== - Coverage 91.6% 91.56% -0.05%  ========================================== Files 153 153 Lines 51273 51275 +2 ========================================== - Hits 46970 46949 -21  - Misses 4303 4326 +23
Flag Coverage Δ
#multiple 89.42% <100%> (-0.03%) ⬇️
#single 40.68% <0%> (-0.11%) ⬇️
Impacted Files Coverage Δ
pandas/core/resample.py 96.35% <100%> (+0.01%) ⬆️
pandas/io/gbq.py 25% <0%> (-58.34%) ⬇️
pandas/plotting/_converter.py 64.78% <0%> (-1.74%) ⬇️
pandas/util/testing.py 81.82% <0%> (-0.2%) ⬇️
pandas/core/frame.py 97.81% <0%> (-0.1%) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 34a8d36...77cba2b. Read the comment docs.
@jreback jreback added Reshaping Concat, Merge/Join, Stack/Unstack, Explode Timedelta Timedelta data type labels Dec 10, 2017
@jreback
Copy link
Contributor

jreback commented Dec 10, 2017

can you also test for some invalid offsets
@jreback
Copy link
Contributor

jreback commented Jan 21, 2018

closing as stale. if you want to continue working, pls ping.
@jreback jreback closed this Jan 21, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Reshaping Concat, Merge/Join, Stack/Unstack, Explode Timedelta Timedelta data type

3 participants

@Licht-T @pep8speaks @jreback