15819 rolling window on empty df #16431

chernrick · 2017-05-22T19:46:27Z

closes ERR: consistency in rolling on empty frames #15819
tests added / passed
passes git diff upstream/master --name-only -- '*.py' | flake8 --diff
whatsnew entry

TomAugspurger

Looks good, just a couple small comments.

TomAugspurger · 2017-05-22T19:59:43Z

doc/source/whatsnew/v0.20.2.txt

@@ -37,6 +37,7 @@ Bug Fixes
 ~~~~~~~~~

 - Bug in using ``pathlib.Path`` or ``py.path.local`` objects with io functions (:issue:`16291`)
+- Bug creating rolling datetime rolling window on empty DataFrame (:issue:`15819`)


I think "datetime rolling window" (bit of a weird name).

TomAugspurger · 2017-05-22T20:02:54Z

pandas/tests/test_window.py

@@ -441,6 +441,16 @@ def test_closed(self):
        with pytest.raises(ValueError):
            df.rolling(window=3, closed='neither')

+    def test_empty_df_datetime_rolling_sum(self):
+        result = DataFrame().rolling('1s').sum()


Cna you leave a comment here with the github issue number?

Ok, I have updated with a comment

codecov · 2017-05-22T22:41:24Z

Codecov Report

Merging #16431 into master will decrease coverage by <.01%.
The diff coverage is 100%.

@@            Coverage Diff             @@
##           master   #16431      +/-   ##
==========================================
- Coverage   90.42%   90.42%   -0.01%     
==========================================
  Files         161      161              
  Lines       51023    51023              
==========================================
- Hits        46138    46137       -1     
- Misses       4885     4886       +1

Flag	Coverage Δ
#multiple	`88.26% <100%> (-0.01%)`	⬇️
#single	`40.17% <0%> (ø)`	⬆️

Impacted Files	Coverage Δ
pandas/core/window.py	`96.24% <100%> (ø)`	⬆️
pandas/core/common.py	`91.05% <0%> (-0.34%)`	⬇️

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 49ec31b...d31b983. Read the comment docs.

codecov · 2017-05-22T22:41:33Z

Codecov Report

Merging #16431 into master will increase coverage by <.01%.
The diff coverage is 100%.

@@            Coverage Diff             @@
##           master   #16431      +/-   ##
==========================================
+ Coverage   90.42%   90.43%   +<.01%     
==========================================
  Files         161      161              
  Lines       51023    51036      +13     
==========================================
+ Hits        46138    46152      +14     
+ Misses       4885     4884       -1

Flag	Coverage Δ
#multiple	`88.26% <100%> (ø)`	⬆️
#single	`40.17% <0%> (-0.01%)`	⬇️

Impacted Files	Coverage Δ
pandas/core/window.py	`96.48% <100%> (+0.23%)`	⬆️
pandas/io/feather_format.py	`85.71% <0%> (-2.17%)`	⬇️
pandas/core/reshape/merge.py	`94.18% <0%> (-0.01%)`	⬇️
pandas/errors/__init__.py	`100% <0%> (ø)`	⬆️
pandas/core/frame.py	`97.69% <0%> (ø)`	⬆️
pandas/core/missing.py	`84.27% <0%> (ø)`	⬆️
pandas/core/generic.py	`92.13% <0%> (ø)`	⬆️
pandas/core/reshape/reshape.py	`99.28% <0%> (ø)`	⬆️
pandas/io/formats/style.py	`95.92% <0%> (+0.01%)`	⬆️
pandas/io/parsers.py	`95.33% <0%> (+0.01%)`	⬆️
... and 4 more

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 49ec31b...4a81a5e. Read the comment docs.

jreback · 2017-05-23T00:20:17Z

pandas/tests/test_window.py

+
+    def test_empty_df_integer_rolling_sum(self):
+        # Verifies that integer rolling window can be applied to empty DataFrame
+        # GH 15819


can you add the same tests for expanding (not that the expanding will raise for the time-aware, a separate issue, so xfail that one, put a ref to #16425 there).

jreback · 2017-05-23T00:21:44Z

pandas/tests/test_window.py

+
+    def test_empty_df_integer_rolling_sum(self):
+        # Verifies that integer rolling window can be applied to empty DataFrame
+        # GH 15819


can you add these as well (which are known working)

DataFrame(index=pd.DatetimeIndex([])).rolling(1).sum() DataFrame(index=pd.DatetimeIndex([])).rolling('1s').sum()

jreback · 2017-05-23T00:22:14Z

doc/source/whatsnew/v0.20.2.txt

@@ -37,6 +37,7 @@ Bug Fixes
 ~~~~~~~~~

 - Bug in using ``pathlib.Path`` or ``py.path.local`` objects with io functions (:issue:`16291`)
+- Bug creating datetime rolling window on empty DataFrame (:issue:`15819`)


can you move to the rolling bug fix section

…g window section

…2 tests

chernrick · 2017-05-23T08:24:44Z

@jreback Thanks for the review - I have tried to address your comments

jreback · 2017-05-23T10:32:45Z

doc/source/whatsnew/v0.20.2.txt

@@ -68,7 +69,7 @@ Plotting
 Groupby/Resample/Rolling
 ^^^^^^^^^^^^^^^^^^^^^^^^

-
+- Bug creating datetime rolling window on empty DataFrame (:issue:`15819`)


on an empty DataFrame

jreback · 2017-05-23T10:35:13Z

pandas/tests/test_window.py

@@ -441,6 +441,34 @@ def test_closed(self):
        with pytest.raises(ValueError):
            df.rolling(window=3, closed='neither')

+    def test_empty_df_datetime_rolling_sum(self):


rather than making all separate tests, either use parametrize, or you can consolidate these into 2 tests for rolling (and 2 for expanding)

e.g.

@pytest.mark.parametrize('roller', ['1s', 1]): def tests_empty_df_rolling(self, roller): expected = DataFrame() result = DataFrame().rolling(roller).sum() tm.assert_frame_equal(result, expected) expected = DataFrame(index=pd.DatetimeIndex([])) result = DataFrame(index=pd.DatetimeIndex([])).rolling(roller).sum() tm.assert_frame_equal(result, expected)

chernrick · 2017-05-24T01:20:45Z

@jreback I have tried to address your comments. Thanks

jreback · 2017-05-24T12:03:01Z

pandas/tests/test_window.py

@@ -483,6 +496,35 @@ def test_numpy_compat(self):
            tm.assert_raises_regex(UnsupportedFunctionCall, msg,
                                   getattr(e, func), dtype=np.float64)

+    # TODO: GH 16425: Add '1s' datetime expander when GH 16425 is resolved


so you can add in a parameter that xfails itself, IOW

@pytest.mark.parametrize('expander', [1, pytest.mark.xfail(reason='GH 16425 expanding with offset not supported)('1s')])

jreback · 2017-05-24T12:03:29Z

pandas/tests/test_window.py

+        result = DataFrame(index=pd.DatetimeIndex([])).expanding(expander).sum()
+        tm.assert_frame_equal(result, expected)
+
+    # TODO: GH 16425: Remove this test and add '1s' to roller parameter of test_empty_df_expanding() parameter


these lines are too long for the linter

jreback · 2017-05-24T12:04:07Z

pandas/tests/test_window.py

+
+    # TODO: GH 16425: Remove this test and add '1s' to roller parameter of test_empty_df_expanding() parameter
+    # when GH 16425 is resolved
+    @pytest.mark.xfail(reason="Open issue GH 16425")


make a referece to what this issue is fixing as well (like I showed above), this is a printed warning when running the tests e.g.

pytest ..... -r x will show them

jreback

minor doc/test comments. some linting issues. ping when green.

chernrick · 2017-05-25T06:50:39Z

@jreback Thanks for the suggestions. I have updated the tests as you have indicated and verified the new test code lints clean.

jreback · 2017-05-25T10:23:23Z

thanks!

(cherry picked from commit e41fe7f)

chernrick added 2 commits May 22, 2017 12:27

BUG Support empty DataFrame for datetime rolling window

9cbaef6

BUG update release note for issue 15819

d31b983

TomAugspurger added this to the 0.20.2 milestone May 22, 2017

TomAugspurger added Reshaping Concat, Merge/Join, Stack/Unstack, Explode Datetime Datetime data dtype labels May 22, 2017

TomAugspurger reviewed May 22, 2017

View reviewed changes

Address code review comments

5fb1170

jreback reviewed May 23, 2017

View reviewed changes

jreback mentioned this pull request May 23, 2017

ENH: add expanding window time support #16425

Open

jreback reviewed May 23, 2017

View reviewed changes

chernrick added 2 commits May 23, 2017 01:16

Add additional tests from code review and move release note to rollin…

670a33a

…g window section

Separate TestRolling.test_empty_df_datetime_index_rolling_sum() into …

e913664

…2 tests

jreback requested changes May 23, 2017

View reviewed changes

chernrick added 2 commits May 23, 2017 18:17

Combine tests into parameterized tests; update grammar in release note

985522f

Fix typo in comment

6007654

jreback reviewed May 24, 2017

View reviewed changes

jreback approved these changes May 24, 2017

View reviewed changes

Address code review comments

4a81a5e

jreback merged commit e41fe7f into pandas-dev:master May 25, 2017

TomAugspurger pushed a commit to TomAugspurger/pandas that referenced this pull request May 29, 2017

15819 rolling window on empty df (pandas-dev#16431)

c4b53c7

(cherry picked from commit e41fe7f)

TomAugspurger added the Backported label May 30, 2017

TomAugspurger removed the Needs Backport label May 30, 2017

TomAugspurger pushed a commit that referenced this pull request May 30, 2017

15819 rolling window on empty df (#16431)

8ae7fd5

(cherry picked from commit e41fe7f)

stangirala pushed a commit to stangirala/pandas that referenced this pull request Jun 11, 2017

15819 rolling window on empty df (pandas-dev#16431)

522d7f3

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

15819 rolling window on empty df #16431

15819 rolling window on empty df #16431

chernrick commented May 22, 2017

TomAugspurger left a comment

TomAugspurger May 22, 2017

TomAugspurger May 22, 2017

chernrick May 22, 2017

codecov bot commented May 22, 2017

codecov bot commented May 22, 2017 •

edited

Loading

jreback May 23, 2017

jreback May 23, 2017

jreback May 23, 2017

chernrick commented May 23, 2017

jreback May 23, 2017

jreback May 23, 2017

chernrick commented May 24, 2017

jreback May 24, 2017

jreback May 24, 2017

jreback May 24, 2017

jreback left a comment

chernrick commented May 25, 2017

jreback commented May 25, 2017

15819 rolling window on empty df #16431

15819 rolling window on empty df #16431

Conversation

chernrick commented May 22, 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

codecov bot commented May 22, 2017

Codecov Report

codecov bot commented May 22, 2017 • edited Loading

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

chernrick commented May 23, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

chernrick commented May 24, 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

chernrick commented May 25, 2017

jreback commented May 25, 2017

codecov bot commented May 22, 2017 •

edited

Loading