ENH: Allow users to definite their own window bound calculations in rolling #29878

mroeschke · 2019-11-27T03:53:34Z

Currently rolling allows integer and offset as window arguments. This PR allows users to define a custom method for calculating window boundaries by passing a subclass of a new BaseIndexer

from pandas.api.indexers import BaseIndexer

class MyWindowIndexer(BaseIndexer):

    def get_window_bounds(self, ...):
          ....
          return (start, end)

indexer = MyWindowIndexer(arg1, arg2, ...)
result = df.rolling(indexer).mean()

Could help address more custom rolling window operations as described in #26959, #13969, #25510, #24295

…sses

WillAyd

Nice idea. Can you add test(s)?

WillAyd · 2019-11-27T04:21:30Z

pandas/_libs/window/indexers.pyx

+        **kwargs :
+            keyword argument that will be available when get_window_bounds is called
+        """
+        self.__dict__.update(kwargs)


What is this doing?

Yah can we avoid this pattern?

Yeah sorry, it's a bit cryptic.

The motivation is to allow a way for get_window_bounds to access an external variable that may not be passed in rolling or the aggregation function by making it accessible as a class attribute.

For example:

class MyIndexer(BaseIndexer): def get_window_bounds(self, ...): start = self.external_column + min_periods .... indexer = MyIndexer(external_column=df['external_column'])

Could make this pattern in the __init__ more explicit and add validation.

jreback

if u can test with a bad BaseIndexer

eg where get_windows_bounds raises or is missing
we would likely give an odd error message

unit tests otherwise are not needed otherwise except as an explicit smoke test; we are already using this in all rolling tests

jreback · 2019-11-27T05:08:45Z

pandas/_libs/window/indexers.pyx

+        Computes the fixed bounds of a window.
+
+        Parameters
+        ----------


i think could share these docstrings

jreback · 2019-11-27T05:11:44Z

pandas/core/window/rolling.py

        """
        Return an indexer class that will compute the window start and end bounds
        """
+        if isinstance(self.window, window_indexers.BaseIndexer):


technically we don’t actually care if this is a BaseIndexer

rather we care that it has a get_windows_bound with the correct signature

pandas/_libs/window/indexers.pyx

jbrockmendel · 2019-11-27T05:29:51Z

so far the only thing in indexers.pyx for which being cython really matters is VariableWindowIndexer.build, which is a static method, so could just as easily be a module-level function. Do these classes need to be in the cython code?

mroeschke · 2019-11-27T18:05:20Z

Do these classes need to be in the cython code?

Good point @jbrockmendel. I can move these indexers to pandas/core/window/indexers.py and just import "VariableWindowIndexer.build" as a function from pandas/_libs/window/indexers.py

pep8speaks · 2019-11-27T21:36:33Z

Hello @mroeschke! 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-12-04 21:56:57 UTC

jbrockmendel · 2019-11-27T23:01:45Z

pandas/_libs/window/indexers.pyx

+def calculate_variable_window_bounds(
+    int64_t num_values,
+    int64_t window_size,
+    object min_periods,


looks like min_periods is not used. is this a dummy to make the signature match something else? if so, pls comment/docstring

jbrockmendel · 2019-11-27T23:04:45Z

pandas/core/window/rolling.py

@@ -400,13 +421,15 @@ def _get_cython_func_type(self, func):
            self._get_roll_func("{}_fixed".format(func)), win=self._get_window()
        )

-    def _get_window_indexer(self):
+    def _get_window_indexer(self, index_as_array):


can index_as_array or the return type be annotated?

jbrockmendel · 2019-11-27T23:05:41Z

pandas/core/window/indexers.py

+        win_type: Optional[str] = None,
+    ) -> Tuple[np.ndarray, np.ndarray]:
+
+        raise NotImplementedError


AbstractMethodError?

jbrockmendel · 2019-11-27T23:06:29Z

pandas/core/window/indexers.py

+        **kwargs :
+            keyword argument that will be available when get_window_bounds is called
+        """
+        self.index = index


the name index makes me expect a pd.Index. Is there a less ambiguous name?

jreback

looks pretty good. not using ExpandingIndexer right now, correct?

jreback · 2019-12-02T13:14:28Z

pandas/_libs/window/indexers.pyx

+    object center,  # unused but here to match get_window_bounds signature
+    object closed,
+    const int64_t[:] index
+):


can you add a doc-string

jreback · 2019-12-02T13:16:53Z

pandas/core/window/rolling.py

-                    min_periods = calculate_min_periods(
-                        window, self.min_periods, len(x), require_min_periods, floor
+                    if not isinstance(window, BaseIndexer):
+                        min_periods = calculate_min_periods(


can you do these outside of the function? (as they don't depend on anything inside) and makes logic simpler

These both depend on x after the nans are appended from the center calculation.
x = np.concatenate((x, additional_nans))

jreback · 2019-12-02T13:17:47Z

pandas/core/window/rolling.py

-                    start, end = window_indexer(
-                        x, window, self.closed, index_as_array
-                    ).get_window_bounds()
+                    if np.any(np.diff(start) < 0) or np.any(np.diff(end) < 0):


you can just set is_monotonic_bounds=np.any(.......)

and pass that in line 482

Will need to add is_monotonic_bounds as an argument to all the "fixed" functions in Cython (didn't include them originally since the "fixed" function doesn't need this argument), but I suppose that makes this cleaner.

Actually I cannot easily modify this yet without another refactor. func here isn't always a cython function that could accept is_monotonic_bounds; sometimes it's a python wrapped function like apply_func or _zqrt. I can look into this in another refactor.

k, followup is ok

mroeschke · 2019-12-03T06:40:32Z

not using ExpandingIndexer right now, correct?

@jreback. Correct, just exists for testing purposes.

jreback · 2019-12-04T14:25:38Z

pandas/_libs/window/aggregations.pyx

@@ -198,11 +199,10 @@ def roll_sum_variable(ndarray[float64_t] values, ndarray[int64_t] start,
            s = start[i]
            e = end[i]

-            if i == 0:
+            if i == 0 or not is_monotonic_bounds:


could we do a simple calculcate in start/end to determine if is_monotonic is true here? it likely is quite performant.

jreback

lgtm. ping on green.

@TomAugspurger @jorisvandenbossche if any comments

jreback · 2019-12-04T14:26:46Z

pandas/core/window/rolling.py

+            self._validate_get_window_bounds_signature(self.window)
+
+    @staticmethod
+    def _validate_get_window_bounds_signature(window):


if you can type

TomAugspurger

Just a comment on the docs. Happy to defer to others on the implementation.

doc/source/whatsnew/v1.0.0.rst

mroeschke · 2019-12-05T01:08:34Z

All green @jreback

Collecting followups:
#29878 (comment)

jreback · 2019-12-05T12:56:16Z

thanks @mroeschke very nice

…olling (pandas-dev#29878)

gandhis1 · 2022-07-15T00:02:36Z

Apologies for the very late PR comment, but shouldn't rolling() default to the VariableWindowIndexer when the passed-in offset is a variable-frequency offset, rather than throwing as it does now?

mroeschke · 2022-07-15T01:35:51Z

Apologies for the very late PR comment, but shouldn't rolling() default to the VariableWindowIndexer when the passed-in offset is a variable-frequency offset, rather than throwing as it does now?

Could you show an example?

gandhis1 · 2022-07-15T01:53:29Z

Python 3.8.13 (default, Mar 28 2022, 11:38:47) 
Type 'copyright', 'credits' or 'license' for more information
IPython 8.3.0 -- An enhanced Interactive Python. Type '?' for help.

In [1]: import pandas as pd
   ...: from pandas.tseries.holiday import USFederalHolidayCalendar
   ...: 
   ...: s = pd.Series(range(10), pd.date_range("7/1/2022", "7/10/2022"))
   ...: bday = pd.tseries.offsets.CustomBusinessDay(n=1, calendar=USFederalHolidayCalendar())
   ...: 

In [2]: s.rolling(bday).median()
ValueError: <CustomBusinessDay> is a non-fixed frequency

In [3]: s.rolling(pd.api.indexers.VariableOffsetWindowIndexer(index=s.index, offset=bday)).median()
Out[3]: 
2022-07-01    0.0
2022-07-02    1.0
2022-07-03    1.5
2022-07-04    2.0
2022-07-05    2.5
2022-07-06    5.0
2022-07-07    6.0
2022-07-08    7.0
2022-07-09    8.0
2022-07-10    8.5
Freq: D, dtype: float64

mroeschke · 2022-07-15T01:57:47Z

VariableOffsetWindowIndexer is not as performant as the prior implementation for fixed offsets, so I don't think the default could be switched. VariableOffsetWindowIndexer was an enhancement to support non-fixed offsets

gandhis1 · 2022-07-15T02:01:34Z

I'm not suggesting changing the default. My suggestion is that if the "X is a non-fixed frequency" exception is being thrown, it has detected that the passed offset is non-fixed, and thus could intelligently fall back to the variable indexer automatically. I'd think a typical user who is passing in a non-fixed offset is doing so deliberately and isn't wanting an exception.

mroeschke · 2022-07-15T02:10:28Z

I think a pull request to improve exception message to suggest using VariableOffsetWindowIndexer would be appreciated. Generally fallback & non-explicit behaviors are preferred to avoid surprises.

gandhis1 · 2022-07-15T03:12:12Z

The code located here just raises:

pandas/pandas/core/window/rolling.py

Lines 1831 to 1838 in 0a8b45f

    
           # this will raise ValueError on non-fixed freqs 
        
           try: 
        
               freq = to_offset(self.window) 
        
           except (TypeError, ValueError) as err: 
        
               raise ValueError( 
        
                   f"passed window {self.window} is not " 
        
                   "compatible with a datetimelike index" 
        
               ) from err

But this could just be another clause right here that sets self.window to a VariableOffsetWindowIndexer that wraps the original self.window, then from then on it's pretty much the same thing as passing in a BaseIndexer, except now the onus isn't on the user to have to do this:

pandas/pandas/core/window/rolling.py

Lines 1859 to 1861 in 0a8b45f

    
           elif isinstance(self.window, BaseIndexer): 
        
               # Passed BaseIndexer subclass should handle all other rolling kwargs 
        
               pass

Matt Roeschke added 6 commits November 24, 2019 22:58

Add BaseIndexer class

1c8c24a

Merge remote-tracking branch 'upstream/master' into feature/baseindexer

1188edd

Reformat Indexers

6b5e894

Remove init

3a310f6

Add BaseIndexer to api and allow rolling to accept BaseIndexer subcla…

d1d0775

…sses

Merge remote-tracking branch 'upstream/master' into feature/baseindexer

218395e

mroeschke added Enhancement Window rolling, ewma, expanding labels Nov 27, 2019

WillAyd requested changes Nov 27, 2019

View reviewed changes

jreback requested changes Nov 27, 2019

View reviewed changes

jbrockmendel reviewed Nov 27, 2019

View reviewed changes

pandas/_libs/window/indexers.pyx Outdated Show resolved Hide resolved

Matt Roeschke added 2 commits November 27, 2019 09:52

Merge remote-tracking branch 'upstream/master' into feature/baseindexer

46d4a52

Lint cython files

c10854d

mroeschke added this to the 1.0 milestone Nov 27, 2019

Matt Roeschke added 5 commits November 27, 2019 12:05

Move indexers to pandas/core/window/indexers

c237090

Share get_window_bounds docstring

1ddc828

isort

a861982

Validate signature of get_window_bounds

8f482f7

Validate signature of get_window_bounds

38691c7

jbrockmendel reviewed Nov 27, 2019

View reviewed changes

Matt Roeschke added 4 commits November 28, 2019 12:21

Merge remote-tracking branch 'upstream/master' into feature/baseindexer

9d740d3

Lint

d18e954

Comment on unused variables in calculate_variable_window_bounds

f06e8e6

Type annotate _get_window_indexer & black

7ccbcd0

jreback requested changes Dec 2, 2019

View reviewed changes

Merge remote-tracking branch 'upstream/master' into feature/baseindexer

0d8cc1f

Matt Roeschke added 2 commits December 2, 2019 23:03

Make doc example a code block, add docstring

25a05fe

Change self.__dict__(kwargs) to more explicit setattr

5d8819f

mroeschke changed the title ~~WIP: ENH: Allow users to definite their own window bound calculations in rolling~~ ENH: Allow users to definite their own window bound calculations in rolling Dec 3, 2019

Matt Roeschke added 3 commits December 3, 2019 10:17

Merge remote-tracking branch 'upstream/master' into feature/baseindexer

9194557

Fix docstring

e7e1061

Merge remote-tracking branch 'upstream/master' into feature/baseindexer

09afec4

jreback reviewed Dec 4, 2019

View reviewed changes

jreback approved these changes Dec 4, 2019

View reviewed changes

TomAugspurger reviewed Dec 4, 2019

View reviewed changes

doc/source/whatsnew/v1.0.0.rst Show resolved Hide resolved

Matt Roeschke added 4 commits December 4, 2019 10:09

Merge remote-tracking branch 'upstream/master' into feature/baseindexer

10c4994

Add BaseIndexer in doc/source/reference/window/rst

9089f7b

Add typing for _validate_get_window_bounds_signature

87e391f

Merge remote-tracking branch 'upstream/master' into feature/baseindexer

7ce1967

jreback merged commit 1f63971 into pandas-dev:master Dec 5, 2019

mroeschke deleted the feature/baseindexer branch December 5, 2019 16:17

mroeschke mentioned this pull request Dec 8, 2019

CLN/TYPE: window aggregation cleanups and typing #30137

Merged

3 tasks

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

ENH: Allow users to definite their own window bound calculations in r…

9a12000

…olling (pandas-dev#29878)

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

ENH: Allow users to definite their own window bound calculations in r…

c894991

…olling (pandas-dev#29878)

ENH: Allow users to definite their own window bound calculations in rolling #29878

ENH: Allow users to definite their own window bound calculations in rolling #29878

Conversation

mroeschke commented Nov 27, 2019 • edited Loading

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

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

jbrockmendel commented Nov 27, 2019

mroeschke commented Nov 27, 2019

pep8speaks commented Nov 27, 2019 • edited Loading

Comment last updated at 2019-12-04 21:56:57 UTC

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

mroeschke Dec 3, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mroeschke commented Dec 3, 2019

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

TomAugspurger left a comment

Choose a reason for hiding this comment

mroeschke commented Dec 5, 2019

jreback commented Dec 5, 2019

gandhis1 commented Jul 15, 2022

mroeschke commented Jul 15, 2022

gandhis1 commented Jul 15, 2022

mroeschke commented Jul 15, 2022

gandhis1 commented Jul 15, 2022 • edited Loading

mroeschke commented Jul 15, 2022

gandhis1 commented Jul 15, 2022

mroeschke commented Nov 27, 2019 •

edited

Loading

pep8speaks commented Nov 27, 2019 •

edited

Loading

mroeschke Dec 3, 2019 •

edited

Loading

gandhis1 commented Jul 15, 2022 •

edited

Loading