BUG: resample by BusinessHour raises ValueError #16447

scari · 2017-05-23T01:18:23Z

closes BUG: resample by BusinessHour raises ValueError #12351
tests added / passed
passes git diff upstream/master --name-only -- '*.py' | flake8 --diff
whatsnew entry

I've been away from this issue for a long time. Just made this PR again at PyCon 2017. Sorry for a long absence.

TomAugspurger

Just a couple style things, and then one question about CustomBusinessHour. Thanks!

TomAugspurger · 2017-05-23T01:24:06Z

pandas/tests/test_resample.py

@@ -2564,6 +2564,22 @@ def test_upsample_daily_business_daily(self):
        expected = ts.asfreq('H', how='s').reindex(exp_rng)
        assert_series_equal(result, expected)

+


May need to remove this like (try flake8 pandas/tests/test_resample.py)

TomAugspurger · 2017-05-23T01:24:22Z

pandas/tests/test_resample.py

@@ -2564,6 +2564,22 @@ def test_upsample_daily_business_daily(self):
        expected = ts.asfreq('H', how='s').reindex(exp_rng)
        assert_series_equal(result, expected)

+
+    # GH12351


This comment can go just below the def test_

TomAugspurger · 2017-05-23T01:27:22Z

pandas/tests/test_resample.py

+        expected_ts = Series(1, index=expected_rng)
+
+        assert_series_equal(result, expected_ts)
+


Could you add a test against a CustomBusinessHour (I've never used one, but can help figure it out if you need).

I just added it. Please take a look if it's okay.

jreback · 2017-05-23T10:47:57Z

pandas/core/resample.py

-
-    if not isinstance(offset, Tick):  # and first.time() != last.time():
+    # GH12351
+    elif isinstance(offset, BusinessHour) or isinstance(offset, CustomBusinessHour):


this is pretty hacky (well it was a hack). anything more general to do here?

The special casing of BusinessHour, or the two isinstance checks? The two checks could be replaced with

elif issubclass(type(offset), BusinessHourMixin): ...

but that still feels a bit hacky :)

I replaced it with @TomAugspurger suggested.

TomAugspurger · 2017-05-23T19:44:12Z

New test looks good, seems to have some style errors though:

pandas/tests/test_resample.py:2573:37: E128 continuation line under-indented for visual indent
pandas/tests/test_resample.py:2574:37: E128 continuation line under-indented for visual indent
pandas/tests/test_resample.py:2589:37: E128 continuation line under-indented for visual indent
pandas/tests/test_resample.py:2590:37: E128 continuation line under-indented for visual indent
pandas/tests/test_resample.py:2593:9: E303 too many blank lines (2)

Just need to add one more space, so that block lines up vertically. Also can remove the extra blank line on 2593.

codecov · 2017-05-23T19:53:28Z

Codecov Report

❗ No coverage uploaded for pull request base (master@04356a8). Click here to learn what that means.
The diff coverage is 50%.

@@            Coverage Diff            @@
##             master   #16447   +/-   ##
=========================================
  Coverage          ?   90.42%           
=========================================
  Files             ?      161           
  Lines             ?    51025           
  Branches          ?        0           
=========================================
  Hits              ?    46138           
  Misses            ?     4887           
  Partials          ?        0

Flag	Coverage Δ
#multiple	`88.26% <50%> (?)`
#single	`40.17% <25%> (?)`

Impacted Files	Coverage Δ
pandas/core/resample.py	`95.75% <50%> (ø)`

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 04356a8...f355763. Read the comment docs.

codecov · 2017-05-23T19:53:35Z

Codecov Report

Merging #16447 into master will decrease coverage by <.01%.
The diff coverage is 50%.

@@            Coverage Diff             @@
##           master   #16447      +/-   ##
==========================================
- Coverage    90.4%    90.4%   -0.01%     
==========================================
  Files         161      161              
  Lines       51033    51035       +2     
==========================================
  Hits        46136    46136              
- Misses       4897     4899       +2

Flag	Coverage Δ
#multiple	`88.23% <50%> (-0.01%)`	⬇️
#single	`40.17% <25%> (-0.01%)`	⬇️

Impacted Files	Coverage Δ
pandas/core/resample.py	`95.75% <50%> (-0.33%)`	⬇️

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 97ad3fb...6f03530. Read the comment docs.

scari · 2017-05-23T19:55:41Z

Thanks, @TomAugspurger I just cleaned up you mention. ;)

closes pandas-dev#12351

follow up to GH16447

jreback · 2017-05-24T23:10:29Z

pandas/core/resample.py

@@ -1271,8 +1272,11 @@ def _get_range_edges(first, last, offset, closed='left', base=0):
        if (is_day and day_nanos % offset.nanos == 0) or not is_day:
            return _adjust_dates_anchored(first, last, offset,
                                          closed=closed, base=base)
-
-    if not isinstance(offset, Tick):  # and first.time() != last.time():
+    # GH12351


not what I meant. This is not a general fix at all. This is the same as before. The question is what kind of offset needs to do this kind of rollback / normalization.

got your point. so do we need to generalize the edges detection here?

jreback · 2017-05-24T23:11:09Z

pandas/tests/test_resample.py

+        # GH12351
+        rng = pd.date_range(start='2017-05-18 00:00:00',
+                            end='2017-05-19 23:00:00',
+                            freq='H')


these are asfreq tests, NOT resample tests.

jreback

need some tests from the original issue.

jreback · 2017-08-17T10:23:21Z

if you want to try to generalize can have another look.

jreback · 2017-09-23T17:06:55Z

closing. I think this needs a bit more generalization. If you want to continue, pls ping.

scari · 2017-10-10T12:29:20Z

I'd like to continue for this Oct. I have some time to do now. Where can I start with? re-open this issue? Would you guide me? @jreback

jreback · 2017-11-12T19:11:18Z

can you rebase & move notes to 0.22.0

jreback · 2017-12-10T23:32:56Z

closing as stale, but if you'd like to keep working, ping and we can reopen

TomAugspurger reviewed May 23, 2017

View reviewed changes

TomAugspurger added the Resample resample method label May 23, 2017

TomAugspurger added this to the 0.21.0 milestone May 23, 2017

jreback reviewed May 23, 2017

View reviewed changes

scari force-pushed the issue-12351 branch from 81a4531 to b7f5079 Compare May 23, 2017 18:45

scari force-pushed the issue-12351 branch from b7f5079 to f355763 Compare May 23, 2017 19:53

TomAugspurger approved these changes May 23, 2017

View reviewed changes

Younggun Kim added 2 commits May 24, 2017 12:06

BUG: resample by BusinessHour raises ValueError

923507f

closes pandas-dev#12351

TST: Test CustomBusinessHour resampling (pandas-dev#12351)

6f03530

follow up to GH16447

scari force-pushed the issue-12351 branch from f355763 to 6f03530 Compare May 24, 2017 19:06

jreback reviewed May 24, 2017

View reviewed changes

jreback requested changes May 24, 2017

View reviewed changes

jreback removed this from the 0.21.0 milestone Aug 17, 2017

jreback closed this Sep 23, 2017

jreback reopened this Oct 10, 2017

scari mentioned this pull request Oct 11, 2017

BUG: resample by BusinessHour raises ValueError #12351

Open

jreback closed this Dec 10, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

BUG: resample by BusinessHour raises ValueError #16447

BUG: resample by BusinessHour raises ValueError #16447

scari commented May 23, 2017

TomAugspurger left a comment

TomAugspurger May 23, 2017

TomAugspurger May 23, 2017

TomAugspurger May 23, 2017

scari May 23, 2017

jreback May 23, 2017

TomAugspurger May 23, 2017 •

edited

Loading

scari May 23, 2017

TomAugspurger commented May 23, 2017

codecov bot commented May 23, 2017

codecov bot commented May 23, 2017 •

edited

Loading

scari commented May 23, 2017

jreback May 24, 2017

scari May 29, 2017

jreback May 24, 2017

jreback left a comment

jreback commented Aug 17, 2017

jreback commented Sep 23, 2017

scari commented Oct 10, 2017

jreback commented Nov 12, 2017

jreback commented Dec 10, 2017

		@@ -2564,6 +2564,22 @@ def test_upsample_daily_business_daily(self):
		expected = ts.asfreq('H', how='s').reindex(exp_rng)
		assert_series_equal(result, expected)

		expected_ts = Series(1, index=expected_rng)

		assert_series_equal(result, expected_ts)

BUG: resample by BusinessHour raises ValueError #16447

BUG: resample by BusinessHour raises ValueError #16447

Conversation

scari commented May 23, 2017

TomAugspurger left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

TomAugspurger May 23, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

TomAugspurger commented May 23, 2017

codecov bot commented May 23, 2017

Codecov Report

codecov bot commented May 23, 2017 • edited Loading

Codecov Report

scari commented May 23, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jreback left a comment

Choose a reason for hiding this comment

jreback commented Aug 17, 2017

jreback commented Sep 23, 2017

scari commented Oct 10, 2017

jreback commented Nov 12, 2017

jreback commented Dec 10, 2017

TomAugspurger May 23, 2017 •

edited

Loading

codecov bot commented May 23, 2017 •

edited

Loading