Center rolling window for time offset #38780

luholgit · 2020-12-29T14:59:14Z

closes Center rolling window with date NotImplementedError #20012
tests added / passed
passes black pandas
passes git diff upstream/master -u -- "*.py" | flake8 --diff
whatsnew entry

Added center functionality for VariableWindowIndexer. Note - I am unsure if the NotImplementedError in lines 1966-1969 in rolling.py still correctly raises an error for offset based windows.

Finalizes previous PR #36097

…nter_window

jreback

needs tests

luholgit · 2020-12-29T16:21:19Z

take

mroeschke · 2020-12-29T20:16:41Z

pandas/_libs/window/indexers.pyx

@@ -11,7 +11,7 @@ def calculate_variable_window_bounds(
    int64_t num_values,
    int64_t window_size,
    object min_periods,  # unused but here to match get_window_bounds signature
-    object center,  # unused but here to match get_window_bounds signature
+    object center,


Should be bint center now if supported

thanks for your quick response, this is done now. :)

mroeschke · 2020-12-29T20:18:14Z

pandas/core/window/rolling.py

@@ -450,7 +457,36 @@ def homogeneous_func(values: np.ndarray):
            if values.size == 0:
                return values.copy()

+            def _calculate_center_offset(window) -> int:


None of this code below should be needed.

The center calculation should just adjust the indexers return in calculate_variable_window_bounds and everything else should work correctly.

mroeschke · 2020-12-29T20:19:44Z

pandas/core/window/rolling.py

@@ -334,6 +334,13 @@ def _get_window_indexer(self) -> BaseIndexer:
        """
        if isinstance(self.window, BaseIndexer):
            return self.window
+
+        if self.is_freq_type:


This clause shouldn't be needed either. self,center should just be passed to VariableWindowIndexer in the clause below

this is corrected too

mroeschke · 2020-12-29T20:20:20Z

pandas/core/window/rolling.py

            or isinstance(
                self._on, (ABCDatetimeIndex, ABCTimedeltaIndex, ABCPeriodIndex)
            )
        ) and isinstance(self.window, (str, BaseOffset, timedelta)):

            self._validate_monotonic()

-            # we don't allow center
-            if self.center:
+            # we don't allow center for offset based windows


This entire clause should be removed once center is supported

removed this now, too! :)

arw2019 · 2020-12-29T20:54:11Z

pandas/core/window/rolling.py

+                _calculate_center_offset(self.window)
+                if self.center
+                and not isinstance(
+                    self._get_window_indexer(self.window), VariableWindowIndexer


I don't think you should be passing an argument here, just self._get_window_indexer()

I removed the argument here, too. Thanks for the feedback!

jreback

you need tests to validate each path that is added.

jreback · 2020-12-30T14:27:25Z

pandas/_libs/window/indexers.pyx

@@ -74,14 +78,27 @@ def calculate_variable_window_bounds(
    # right endpoint is open
    else:
        end[0] = 0
+    if center_window:
+        for j in range(0, num_values+1):
+            if (index[j] == index[0] + index_growth_sign*window_size/2 and


use spaces around operators & parens to format this (we don't lint these files automatically)

luholgit · 2021-01-05T16:07:25Z

pandas/core/window/rolling.py

+    def _center_window(self, result: np.ndarray, offset: int) -> np.ndarray:
+        """
+        Center the result in the window for weighted rolling aggregations.
+        """
+        if self.axis > result.ndim - 1:
+            raise ValueError("Requested axis is larger then no. of argument dimensions")
+
+        if offset > 0:
+            lead_indexer = [slice(None)] * result.ndim
+            lead_indexer[self.axis] = slice(offset, None)
+            result = np.copy(result[tuple(lead_indexer)])
+        return result
+


In an effort to make the tests run again before introducing new ones, I encountered an error with "'Rolling' object has no attribute '_center_window'".
I tried to solve the error by adding the _center_window() method to the BaseWindow class.
However, this resulted in new errors.

I am not sure whether it is correct to follow this debugging path. Could you quickly comment @jreback , @mroeschke or @arw2019?

Meanwhile, I am working on adding the changes you asked for. :)

As mentioned in #38780 (comment), I think you shouldn't need any additional code in this _apply method.

The changes should just need to occur in pandas/_libs/window/indexers.pyx in the calculate_variable_window_bounds function to make it return indexes that "center" the window.

For example, for non time offests, center is handled when calculating the indexes of each window

pandas/pandas/core/window/indexers.py

Line 81 in 67d4cae

if center:

As mentioned in #38780 (comment), I think you shouldn't need any additional code in this _apply method.

The changes should just need to occur in pandas/_libs/window/indexers.pyx in the calculate_variable_window_bounds function to make it return indexes that "center" the window.

Just removed the unnecessary lines from rolling.py and that made the tests work again, thanks for that hint! At first I understood I had to migrate those lines to the pandas/_libs/window/indexers.pyx instead of ditching them all together.

mroeschke · 2021-01-06T18:26:18Z

pandas/_libs/window/indexers.pyx

@@ -60,6 +62,8 @@ def calculate_variable_window_bounds(

    if index[num_values - 1] < index[0]:
        index_growth_sign = -1
+    if center:


Sine we typed center as bint. This isnt needed anymore

mroeschke · 2021-01-06T18:26:34Z

pandas/core/window/rolling.py

@@ -334,9 +334,12 @@ def _get_window_indexer(self) -> BaseIndexer:
        """
        if isinstance(self.window, BaseIndexer):
            return self.window
+


Can you undo this white space?

mroeschke · 2021-01-06T18:26:38Z

pandas/core/window/rolling.py

@@ -451,6 +454,7 @@ def homogeneous_func(values: np.ndarray):
                return values.copy()

            def calc(x):
+


Can you undo this white space?

mroeschke · 2021-01-06T18:26:49Z

pandas/core/window/rolling.py

@@ -1880,20 +1884,14 @@ def validate(self):
        # we allow rolling on a datetimelike index
        if (
            self.obj.empty
+            # TODO: add "or self.is_datetimelike"?


Don't need this TODO

jreback

@luholgit we need actual tests that lock down whether the bounds are correct including variable size windows. There are some center tests for fixed windows, follow those examples.

pep8speaks · 2021-03-29T12:17:13Z

Hello @luholgit! 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 2021-04-09 12:15:28 UTC

jreback

pls also add a whatsnew note (enh is ok), as well as see if any updates are needed in the https://pandas.pydata.org/pandas-docs/dev/user_guide/window.html, also the doc-string of rolling might need a small update as well

pandas/_libs/window/indexers.pyx

pandas/tests/window/test_rolling.py

jreback · 2021-04-02T18:07:07Z

pandas/tests/window/test_rolling.py


    tm.assert_frame_equal(result, expected)


-def test_closed_fixed_binary_col():
+def test_datetimelike_centered_selections(closed, arithmetic_win_operators):


can you paramterize on closed (e.g. directly put your window selection up there with the closed parameter), eg.. don't use the fixture

Good idea, this is done, too.

jreback · 2021-04-02T18:07:35Z

pandas/tests/window/test_rolling.py

+            + [77.0] * 2
+        )
+
+    else:


if doing this pls paramterize outside the function

jreback · 2021-04-07T13:48:17Z

doc/source/whatsnew/v1.3.0.rst

+used (:issue:`38780`).
+For example:
+
+.. code-block:: ipython


use ipython:: python so the code runs (don't need the prompts or output just the code)

jreback · 2021-04-07T13:48:55Z

doc/source/whatsnew/v1.3.0.rst

@@ -134,6 +134,30 @@ a copy will no longer be made (:issue:`32960`)
 The default behavior when not passing ``copy`` will remain unchanged, i.e.
 a copy will be made.

+Centered Datetime-Like Rolling Windows


would not object to an example in window.rst as well

I added the same example also to the window.rst now

pandas/_libs/window/indexers.pyx

mroeschke · 2021-04-07T16:49:33Z

doc/source/whatsnew/v1.3.0.rst

+              index=date_range("2020", periods=5, freq="1D")
+           )
+
+   In [2]: df.rolling("2D", center=True).mean()


Don't need the In Out annotations when using .. ipython:: python (the code will execute line by line

luholgit · 2021-04-08T11:45:09Z

pandas/tests/window/test_rolling.py

+def test_even_number_window_alignment():
+    # see discussion in GH 38780
+    s = Series(range(3), index=date_range(start="2020-01-01", freq="D", periods=3))
+
+    # behavior of index- and datetime-based windows differs here!
+    # s.rolling(window=2, min_periods=1, center=True).mean()
+
+    result = s.rolling(window="2D", min_periods=1, center=True).mean()
+
+    expected = Series([0.5, 1.5, 2], index=s.index)
+
+    tm.assert_series_equal(result, expected)


@mroeschke I added this test to check for the behavior when operating with even sized windows. As mentioned by @sevberg this currently does behave differently than the classic index-based windows.

Is that what you had in mind here?

This test would also work with N instead of D as frequency, therefore I am not sure if this is what you meant when referring to nano seconds?

Yes I believe this test exercises the difference in behavior which is good.

Great, thanks!

jreback

just a minor doc comment, ping on green (I think you addressed all of @mroeschke comments but if not pls do)

doc/source/user_guide/window.rst

jreback · 2021-04-08T20:19:01Z

doc/source/whatsnew/v1.3.0.rst

+
+    df = pd.DataFrame(
+        {"A": [0, 1, 2, 3, 4]}, index=pd.date_range("2020", periods=5, freq="1D")
+    )


same comment as above

jreback · 2021-04-08T20:19:19Z

doc/source/user_guide/window.rst

@@ -156,6 +156,16 @@ By default the labels are set to the right edge of the window, but a
   s.rolling(window=5).mean()
   s.rolling(window=5, center=True).mean()

+This can also be applied to datetime-like indices.
+


can you add a versionadded 1.3 tag here

jreback · 2021-04-09T16:28:17Z

thanks @luholgit very nice!

luholgit · 2021-04-13T12:11:23Z

Big thanks to you, @jreback and @mroeschke! I've learned a lot while working on this. :)

adamamiller and others added 5 commits September 3, 2020 09:40

update syntax for pandas style

11c1bb7

Merge branch 'master' of https://github.com/pandas-dev/pandas into ce…

3cac891

…nter_window

fix syntax error

c73ffc4

Merge branch 'master' of github.com:pandas-dev/pandas into center_window

58a8ebf

reintroduce calculate_center_offset as private function

dca9d04

jreback requested changes Dec 29, 2020

View reviewed changes

jreback added the Window rolling, ewma, expanding label Dec 29, 2020

fix double declaration of index_growth_sign

37cb6fa

github-actions bot assigned luholgit Dec 29, 2020

mroeschke reviewed Dec 29, 2020

View reviewed changes

arw2019 reviewed Dec 29, 2020

View reviewed changes

jreback changed the title ~~Center window~~ Center rolling window for time offset Dec 29, 2020

luholacc added 2 commits December 30, 2020 13:48

apply review suggestions

81e0e4e

remove unneccessary exception

463c7f0

jreback requested changes Dec 30, 2020

View reviewed changes

add method "_center_window" to class BaseWindow

321f07c

luholgit commented Jan 5, 2021

View reviewed changes

luholacc added 2 commits January 5, 2021 18:26

use spaces around operators

9270bab

remove unneccessary calculations from rolling.py, tests work again

dd33e32

mroeschke reviewed Jan 6, 2021

View reviewed changes

luholacc added 2 commits January 7, 2021 11:22

remove "test_invalid_center_datetimelike"

6e4da84

remove white spaces and TODO

e0966e8

jreback requested changes Jan 7, 2021

View reviewed changes

luholacc added 4 commits March 29, 2021 14:19

correct too many blank lines

5b9b8ff

fix wrong var name after merge

fca3b4d

remove unused type: ignore

6c1c58a

Merge branch 'master' of github.com:pandas-dev/pandas into center_window

b7e5035

jreback requested changes Apr 2, 2021

View reviewed changes

luholacc and others added 5 commits April 6, 2021 09:56

fix datatype in docstring (now bool)

f44c6e6

dd parametrize for window_selections

3dcad64

black formatting

0f9f6df

parametrize "test_rolling_window_as_string" outside of function

315b320

add whatsnew note

f7d1110

jreback added this to the 1.3 milestone Apr 7, 2021

jreback requested changes Apr 7, 2021

View reviewed changes

luholgit added 2 commits April 7, 2021 17:13

remove center=False, is already default

fc88ae4

remove prompts and output from whatsnew

e07a1f2

mroeschke reviewed Apr 7, 2021

View reviewed changes

luholgit added 3 commits April 8, 2021 09:48

remove in and out annotations

cefbb16

add datetime-like center example

edbfd21

add test for different behavior of window alignment

bfc0f0d

luholgit commented Apr 8, 2021

View reviewed changes

jreback reviewed Apr 8, 2021

View reviewed changes

luholgit added 3 commits April 9, 2021 13:55

additional output in centering example

43e04ed

additional output in centering example and comparison

1e724dc

add version tag 1.3

47a3b14

jreback approved these changes Apr 9, 2021

View reviewed changes

jreback merged commit 2e341ab into pandas-dev:master Apr 9, 2021

JulianWgs pushed a commit to JulianWgs/pandas that referenced this pull request Jul 3, 2021

Center rolling window for time offset (pandas-dev#38780)

f4c7103

simonjayhawkins mentioned this pull request Jul 28, 2021

BUG: Inconsistent result from centered datetime-like rolling window aggregates #42753

Closed

3 tasks

		@@ -451,6 +454,7 @@ def homogeneous_func(values: np.ndarray):
		return values.copy()

		def calc(x):

Uh oh!

Center rolling window for time offset #38780

Center rolling window for time offset #38780

Uh oh!

Conversation

luholgit commented Dec 29, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jreback left a comment

Choose a reason for hiding this comment

Uh oh!

luholgit commented Dec 29, 2020

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jreback left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jreback left a comment

Choose a reason for hiding this comment

Uh oh!

pep8speaks commented Mar 29, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Comment last updated at 2021-04-09 12:15:28 UTC

Uh oh!

jreback left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

luholgit commented Dec 29, 2020 •

edited

Loading

pep8speaks commented Mar 29, 2021 •

edited

Loading