fix Rolling for multi-index and reversed index. #28297

leftys · 2019-09-05T14:26:07Z

Fix Rolling operation for level of multi-index and descending time index (that is monotonic, but decreasing).

closes ENH: support monotonic_decreasing on rolling #19248
closes rolling( window='10D') does not work for df with MultiIndex #15584
tests passed
passes black pandas
passes git diff upstream/master -u -- "*.py" | flake8 --diff
whatsnew entry

WillAyd · 2019-09-05T15:30:52Z

Does this close a particular issue? Also tests are the most critical part of any PR so would be great if you can add

leftys · 2019-09-05T15:37:09Z

Does this close a particular issue?

I found those two just now:
#19248
#15584

Also tests are the most critical part of any PR so would be great if you can add

Will do.

pep8speaks · 2019-09-06T08:39:09Z

Hello @leftys! Thanks for updating this PR. We checked the lines you've touched for PEP 8 issues, and found:

There are currently no PEP 8 issues detected in this Pull Request. Cheers! 🍻

Comment last updated at 2019-10-19 20:11:05 UTC

leftys · 2019-09-06T16:29:41Z

@WillAyd I added tests and updated whatsnew to reference the two issues.

jreback

looks good generally

pandas/tests/window/test_rolling.py

pandas/tests/window/test_timeseries_window.py

pandas/core/window/rolling.py

jreback · 2019-09-07T17:42:34Z

can you update the top section with the issue references (use a closes #....)

leftys · 2019-09-07T20:11:37Z

@jreback Fixed according to review comments.

WillAyd

lgtm @jreback

WillAyd · 2019-10-11T22:21:24Z

pandas/core/window/rolling.py

        """
-        if not self._on.is_monotonic:
-            formatted = self.on or "index"
+        if not (self._on.is_monotonic_increasing or self._on.is_monotonic_decreasing):


Surprised this doesn't match the previous definition of just is_monotonic. Would welcome that as a follow up to align the logic

You mean updating is_monotonic to return True also for monotonic decreasing? I tried that, but some of its callers rely on the fact that the sequence is also increasing and many tests failed afterwards. So I would rather not do that, especially not in this PR.

jreback · 2019-10-16T12:46:46Z

pandas/tests/window/test_timeseries_window.py

+                [date_range("20190101", periods=3), range(2)], names=["date", "seq"]
+            ),
+        )
+        result = df.rolling("10d", on=df.index.get_level_values("date")).sum()


this is a very odd way of calling this as it should be a scalar. why do you think we should allow this way?

not sure if I understand you. how about passing something like level = 0 to identify the datetime column of multi index to use for comparing rolling window?

right what I mean, is that this doesn't make rolling any more friendly to a Multi-index. we would need to add a level= kwarg that can be used internall to create the on. Would take a followup PR for that.

jreback

minor comments, ping on green

jreback · 2019-10-18T17:36:31Z

pandas/core/window/rolling.py

        elif isinstance(self.obj, ABCDataFrame) and self.on in self.obj.columns:
            return Index(self.obj[self.on])
        else:
            raise ValueError(
                "invalid on specified as {0}, "
-                "must be a column (if DataFrame) "
+                "must be a column (of DataFrame), an Index "


can you update the doc-string (and type annoation if its there) for on in Rolling

jreback · 2019-10-18T17:37:02Z

pandas/tests/window/test_timeseries_window.py

-        # non-monotonic
-        df.index = reversed(df.index.tolist())
+    def test_non_monotonic_on(self):
+        df = DataFrame(


can you add the issue number as a comment

jreback · 2019-10-18T17:37:17Z

pandas/tests/window/test_timeseries_window.py

+        df = DataFrame(
+            {"A": date_range("20130101", periods=5, freq="s"), "B": range(5)}
+        )
+        df.set_index("A", inplace=True)


don't use inplace in tests at all, rather df = df.set_index(....)

jreback · 2019-10-18T17:39:38Z

pandas/tests/window/test_timeseries_window.py

+                [date_range("20190101", periods=3), range(2)], names=["date", "seq"]
+            ),
+        )
+        result = df.rolling("10d", on=df.index.get_level_values("date")).sum()


right what I mean, is that this doesn't make rolling any more friendly to a Multi-index. we would need to add a level= kwarg that can be used internall to create the on. Would take a followup PR for that.

WillAyd · 2019-10-22T01:35:46Z

Thanks @leftys - very nice PR!

roessig · 2019-11-28T09:27:00Z

pandas/tests/window/test_timeseries_window.py

+        df = DataFrame({"column": [3, 4, 4, 2, 1]}, index=reversed(index))
+        result = df.rolling("2s").min()
+        expected = DataFrame(
+            {"column": [3.0, 3.0, 3.0, 2.0, 1.0]}, index=reversed(index)


It seems to me that the 2 seconds time window is not respected, but the minimum is taken over the whole dataframe. To my understanding, the excpected results would be [3, 4, 2, 1, 1] (closed interval limits) or [3, 4, 2, 2, 1] (open interval end). Even if you consider the next 2 seconds in time order, i.e. the other way round, you would have [3, 3, 4, 2, 1].

Same issue found here. By using df.rolling().apply(func), we can print the window inside func and it is the while dataframe till current offset.

You're right, the expected result is wrong. I debugged it a bit and found out that VariableWindowIndexer indeed works only if the index is monotonic and increasing. I will create another PR to finally fix this.

EDIT: Here it comes: #32386

leftys marked this pull request as ready for review September 5, 2019 15:04

Jan Škoda added 2 commits September 5, 2019 17:13

fix Rolling for multi-index and reversed index.

ce0a5e9

fix whatsnew

1e1af78

WillAyd added the Window rolling, ewma, expanding label Sep 5, 2019

Jan Škoda added 3 commits September 6, 2019 10:38

fix rolling on multi-index level

de83db4

add tests

f78cf28

add issues into whatsnew

859bfea

Jan Škoda added 3 commits September 6, 2019 10:40

pep8 corrections

908342d

fix test

bc77e09

fix black

33c3cca

jreback requested changes Sep 7, 2019

View reviewed changes

jreback added the Enhancement label Sep 7, 2019

review fixes

c5a062f

Jan Škoda added 2 commits September 7, 2019 23:21

fix tests

1ec868b

fix tests #2

753d01c

leftys requested a review from jreback September 20, 2019 21:01

WillAyd approved these changes Oct 11, 2019

View reviewed changes

leftys requested a review from WillAyd October 15, 2019 20:14

jreback added this to the 1.0 milestone Oct 16, 2019

jreback requested changes Oct 16, 2019

View reviewed changes

leftys requested a review from jreback October 18, 2019 14:53

jreback approved these changes Oct 18, 2019

View reviewed changes

review fixes

1b46dc5

WillAyd approved these changes Oct 22, 2019

View reviewed changes

WillAyd merged commit d10e25a into pandas-dev:master Oct 22, 2019

HawkinsBA pushed a commit to HawkinsBA/pandas that referenced this pull request Oct 29, 2019

fix Rolling for multi-index and reversed index. (pandas-dev#28297)

4feb8ac

Reksbril pushed a commit to Reksbril/pandas that referenced this pull request Nov 18, 2019

fix Rolling for multi-index and reversed index. (pandas-dev#28297)

de2e992

roessig reviewed Nov 28, 2019

View reviewed changes

oldrichsmejkal mentioned this pull request Dec 12, 2019

forward rolling functions #6772

Closed

proost pushed a commit to proost/pandas that referenced this pull request Dec 19, 2019

fix Rolling for multi-index and reversed index. (pandas-dev#28297)

dd24571

proost pushed a commit to proost/pandas that referenced this pull request Dec 19, 2019

fix Rolling for multi-index and reversed index. (pandas-dev#28297)

13c5b0d

bongolegend pushed a commit to bongolegend/pandas that referenced this pull request Jan 1, 2020

fix Rolling for multi-index and reversed index. (pandas-dev#28297)

3d7f2c0

This was referenced Mar 1, 2020

BUG: Rolling functions doesn't work on decreasing variable index #32385

Closed

BUG: Fix rolling functions with variable windows on decreasing index #32386

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix Rolling for multi-index and reversed index. #28297

fix Rolling for multi-index and reversed index. #28297

leftys commented Sep 5, 2019 •

edited

Loading

WillAyd commented Sep 5, 2019

leftys commented Sep 5, 2019 •

edited

Loading

pep8speaks commented Sep 6, 2019 •

edited

Loading

leftys commented Sep 6, 2019

jreback left a comment

jreback commented Sep 7, 2019

leftys commented Sep 7, 2019

WillAyd left a comment

WillAyd Oct 11, 2019

leftys Oct 15, 2019

jreback Oct 16, 2019

leftys Oct 18, 2019

jreback Oct 18, 2019

leftys Oct 20, 2019

jreback left a comment

jreback Oct 18, 2019

jreback Oct 18, 2019

jreback Oct 18, 2019

jreback Oct 18, 2019

WillAyd commented Oct 22, 2019

roessig Nov 28, 2019

revv00 Feb 16, 2020 •

edited

Loading

leftys Mar 1, 2020 •

edited

Loading

fix Rolling for multi-index and reversed index. #28297

fix Rolling for multi-index and reversed index. #28297

Conversation

leftys commented Sep 5, 2019 • edited Loading

WillAyd commented Sep 5, 2019

leftys commented Sep 5, 2019 • edited Loading

pep8speaks commented Sep 6, 2019 • edited Loading

Comment last updated at 2019-10-19 20:11:05 UTC

leftys commented Sep 6, 2019

jreback left a comment

Choose a reason for hiding this comment

jreback commented Sep 7, 2019

leftys commented Sep 7, 2019

WillAyd 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

Choose a reason for hiding this comment

jreback 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

WillAyd commented Oct 22, 2019

Choose a reason for hiding this comment

revv00 Feb 16, 2020 • edited Loading

Choose a reason for hiding this comment

leftys Mar 1, 2020 • edited Loading

Choose a reason for hiding this comment

leftys commented Sep 5, 2019 •

edited

Loading

leftys commented Sep 5, 2019 •

edited

Loading

pep8speaks commented Sep 6, 2019 •

edited

Loading

revv00 Feb 16, 2020 •

edited

Loading

leftys Mar 1, 2020 •

edited

Loading