ENH: allow exact matches in time-based .rolling() #13968

agraboso · 2016-08-11T19:25:51Z

closes Exact matches in time-based .rolling() #13965
tests added / passed
passes git diff upstream/master | flake8 --diff
whatsnew entry

TODO: update docs.

chrisaycock · 2016-08-11T19:55:09Z

pandas/core/window.py

            # this will raise ValueError on non-fixed freqs
            self.window = freq.nanos
+            if self.left_closed:
+                self.window += 1


To clarify, the logic adds one extra nanosecond to the search space?

AFAIU, offset-based rolling windows rely on the conversion to nanoseconds in the line right above, so adding one not only works but is the optimal strategy — it includes the left endpoint and nothing else (since nanosecond is the smallest time resolution available).

Can you add a test or two with values at finer resolutions than the offset to confirm this? for example

In [11]: df = pd.DataFrame({'B': [1, 1, 1]}, ...: index = [pd.Timestamp('20130101 09:00:30'), ...: pd.Timestamp('20130101 09:01:00'), ...: pd.Timestamp('20130101 09:02:00')]) In [12]: df.rolling('1Min').sum() Out[12]: B 2013-01-01 09:00:30 1.0 2013-01-01 09:01:00 2.0 2013-01-01 09:02:00 1.0

@chris-b1 Added.

chrisaycock · 2016-08-11T20:00:22Z

The code looks pretty good. We'll need documentation to explain what left_closed is (which you list in your TODO).

agraboso · 2016-08-11T20:08:04Z

The code looks pretty good.

Thanks!

We'll need documentation to explain what left_closed is.

Yes, that'd be the lion's share of this.

I'd like to get the opinion of at least a couple of people about the word left_closed. I chose it because allow_exact_matches seemed long and non-obvious to me. But I fear that people not reading the documentation (which would explain that it only applies to offset-based windows) might misinterpret it. Another option I considered was include_start_time.

chrisaycock · 2016-08-11T20:11:18Z

Yes, some sort of include_... might also work. I originally requested allow_exact_matches since that's the parameter in the new pd.merge_asof(), but nobody else seemed to like that suggestion.

agraboso · 2016-08-11T20:15:12Z

I originally requested allow_exact_matches since that's the parameter in the new pd.merge_asof(), but nobody else seemed to like that suggestion.

It might make sense there (I haven't looked at pd.merge_asof() yet), but I find allow_exact_matches very obscure in this context.

codecov-io · 2016-08-11T20:48:51Z

Current coverage is 84.65% (diff: 100%)

Merging #13968 into master will increase coverage by <.01%

@@             master     #13968   diff @@
==========================================
  Files           144        144          
  Lines         51016      51023     +7   
  Methods           0          0          
  Messages          0          0          
  Branches          0          0          
==========================================
+ Hits          43184      43191     +7   
  Misses         7832       7832          
  Partials          0          0

Powered by Codecov. Last update 3ccb501...62ff604

sinhrks · 2016-08-11T21:54:27Z

pandas/core/window.py

@@ -1044,18 +1055,26 @@ def validate(self):
                                          "for datetimelike and offset "
                                          "based windows")

+            if self.left_closed is not None and not is_bool(self.left_closed):


pls add tests for invalid input.

sinhrks · 2016-08-11T21:57:14Z

Pls also add tests with PeriodIndex and TimedeltaIndex (the latter should raise an error)

jreback · 2016-08-11T22:17:47Z

pandas/core/generic.py

@@ -5382,11 +5382,12 @@ def _add_series_or_dataframe_operations(cls):
        from pandas.core import window as rwindow

        @Appender(rwindow.rolling.__doc__)
-        def rolling(self, window, min_periods=None, freq=None, center=False,
-                    win_type=None, on=None, axis=0):
+        def rolling(self, window, min_periods=None, left_closed=None,


move this after on as its a new parameter

jreback · 2016-08-11T22:19:00Z

we have the convention in pd.date_range (and like)

closed : string or None, default None
    Make the interval closed with respect to the given frequency to
    the 'left', 'right', or both sides (None)

This makes more sense to me. default would be right

agraboso · 2016-08-11T22:29:40Z

we have the convention in pd.date_range

Are you speaking in favor of something different from left_closed?

agraboso · 2016-08-11T22:32:36Z

@sinhrks

Pls also add tests with PeriodIndex and TimedeltaIndex (the latter should raise an error)

Not sure what you mean. Could you please be more precise?

jreback · 2016-08-11T22:46:28Z

@agraboso no, I think we should stick with the spelling: closed='left'|'right'|'both'.

agraboso · 2016-08-11T22:53:27Z

@jreback: the problem is that closed=left doesn't make sense with the current implementation, while closed=right is the default and closed=both is what we are trying to achieve.

jorisvandenbossche · 2016-08-11T22:54:49Z

@jreback the issue with following that spelling of closed='left'|'right'|'both' is that left does not really makes sense in the case of rolling I think?
But if it makes sense, then it is indeed maybe good to follow that

EDIT: well, so what @agraboso said :-)

jreback · 2016-08-11T22:58:05Z

only allow right and both then

I think changing to a non standard argument is even more confusing

jorisvandenbossche · 2016-08-11T23:00:32Z

I proposed closed=True|False in #13965 (comment) to keep the keyword name, but closed='right'|'both' (without left) is then maybe better to keep the passed values also consistent.

agraboso · 2016-08-12T00:08:04Z

I'm open to whatever the consensus might be on the name. Github doesn't have any voting system AFAIK, so I'll add comments below for the proposed options and ask people to vote for them. We can, of course, have a discussion afterwards.

agraboso · 2016-08-12T00:09:23Z

Option: allow_exact_matches=False|True, default False. Please like to vote for this option.

agraboso · 2016-08-12T00:09:41Z

Option: left_closed=False|True, default False. Please like to vote for this option.

jreback · 2016-12-20T17:29:31Z

pandas/core/window.py

@@ -377,6 +381,12 @@ class Window(_Window):

    axis : int or string, default 0

+        .. versionadded:: 0.19.0


jreback · 2016-12-20T17:30:15Z

pandas/core/window.py

@@ -455,6 +465,17 @@ class Window(_Window):
    2013-01-01 09:00:05  NaN
    2013-01-01 09:00:06  4.0

+    For time-based windows, it is possible to make the resulting window
+    contain its left edge by setting closed='both'.
+


can you show closed='right', for comparison

jreback · 2016-12-20T17:33:41Z

lgtm (slight doc correction). pls add a whatsnew for 0.20.0

cc @chrisaycock

jreback · 2016-12-30T21:40:13Z

@agraboso pls add a whatsnew for 0.20.0 (other enhancements)

jreback · 2017-02-27T16:01:13Z

can you rebase / update

chrisaycock · 2017-02-27T16:07:20Z

Looking forward to having this added to 0.20.0. It just needs a whatsnew entry plus a fix to the "0.19.0" versionadded entry.

jreback · 2017-02-27T16:09:53Z

yes just needs minor changes. if @agraboso doesn't fix this up in next couple of days I'll fix on merge.

jreback · 2017-02-27T16:10:35Z

actually prob need some slight doc updates as well (doc-string, maybe in computation.rst)

jreback · 2017-03-14T13:48:20Z

@chrisaycock any other doc updates needed here? (e.g. maybe in merge.rst)?

chrisaycock · 2017-03-14T14:10:14Z

It still needs the versionadded update, the whatsnew entry, and possibly something in computation.rst. I doubt there should be an entry in merge.rst.

jreback · 2017-03-14T14:27:14Z

@chrisaycock sorry, yes, meant computation.rst

jreback · 2017-03-14T14:27:28Z

@agraboso if you can update we can get this in.

carlosdanielcsantos · 2017-03-15T12:01:14Z

I do think the closed='left' makes sense as well. I am developing a project where I am facing the problem of getting rolling time-window statistics for each row without including the current row in the calculations. For me to actual get to this result using the result of rolling() is becoming a nightmare so I was already thinking that changing some lines of code inside pandas would probably be a lot easier.

jreback · 2017-03-15T13:15:32Z

@carlosdanielcsantos so we have closed='right'|'both', are you advocating for adding closed='left' as well? (meaning NOT include the rhs endpoint).?

carlosdanielcsantos · 2017-03-15T14:58:25Z

@jreback If by not including the rhs endpoint you mean not including the "right side" endpoint, yes. As in ti <= t < tf. For the sake of completeness, we could add the option to not include any endpoint at all, ti < t < tf, with something like closed='none'.

Here is a quick example of what it would look like

df = pd.DataFrame({'x': [1]*5},
....:                 index = [pd.Timestamp('20130101 09:00:01'),
....:                          pd.Timestamp('20130101 09:00:02'),
....:                          pd.Timestamp('20130101 09:00:03'),
....:                          pd.Timestamp('20130101 09:00:04'),
....:                          pd.Timestamp('20130101 09:00:06')])

df["right"] = df.rolling('2s', closed='right').x.sum()   # current behavior
df["both"] = df.rolling('2s', closed='both').x.sum()
df["left"] = df.rolling('2s', closed='left').x.sum()
df["open"] = df.rolling('2s', closed='none').x.sum()

print(df)
                     x  right  both  left  open
2013-01-01 09:00:01  1    1.0   1.0   NaN   NaN
2013-01-01 09:00:02  1    2.0   2.0   1.0   1.0
2013-01-01 09:00:03  1    2.0   3.0   2.0   1.0
2013-01-01 09:00:04  1    2.0   3.0   2.0   1.0
2013-01-01 09:00:06  1    1.0   2.0   1.0   NaN

jreback · 2017-03-15T15:02:06Z

@carlosdanielcsantos aside from none -> neither that might be reasonable.

@chrisaycock @agraboso thoughts?

chrisaycock · 2017-03-15T15:14:05Z

I could definitely see closed as 'neither', 'left', 'right', or 'both'.

jreback · 2017-03-15T15:39:08Z

I think then we should directly push closed parameter to cython and change to handle these 4 different cases.

Its possible to do shifting on the window (as is done here), and actually shift the data (to make 'left' and 'neither' work), but I think this will be more transparent.

carlosdanielcsantos · 2017-03-17T18:59:48Z

@jreback sounds good to me. I already started working on it and got the logic right on the code you pointed out. I hope @agraboso doesn't mind if I pick some lines from here to my branch.
Looking at it now, I don't find a reason why this feature should limit itself to time-based windows. Although you can get the 'both' case by increasing the window size, the 'left' and 'neither' cases would be a nice addition to situations where you want to ignore the right endpoint, which is currently not possible.

jreback · 2017-03-17T19:15:11Z

@carlosdanielcsantos ok by me!

sure, on non-time based are ok as well. you'll want to add a sub-section to whatsnew showing this (as well as a similar section in the docs themselves & doc-strings).

agraboso · 2017-03-17T21:04:14Z

@carlosdanielcsantos I don't mind at all. Hack away!

jreback · 2017-03-23T13:21:05Z

@carlosdanielcsantos any luck with this?

carlosdanielcsantos · 2017-03-23T16:12:05Z

@jreback already wrote tests and the code on window.pyx. Now trying to figure out the point to which I need to backpropagate the closed argument in window.py. Some thoughts on this?

jreback · 2017-03-23T19:34:19Z

which I need to backpropagate the closed argument in window.py. Some thoughts on this?

do you have a branch that you can post

carlosdanielcsantos · 2017-03-23T22:13:56Z

@jreback just commited the last changes in which the feature is working for time-based windows (should be quite easy now for fixed windows). Take a look at the code here and let me know what should improved.

jreback · 2017-03-23T22:22:57Z

@carlosdanielcsantos sure, why don't you make a PR (easier to comment on). at a quick glance looks reasonable.

jreback · 2017-03-23T22:38:28Z

superseded by #15795

agraboso mentioned this pull request Aug 11, 2016

Exact matches in time-based .rolling() #13965

Closed

agraboso changed the title ~~EHN: allow exact matches in time-based .rolling()~~ ENH: allow exact matches in time-based .rolling() Aug 11, 2016

chrisaycock reviewed Aug 11, 2016
View reviewed changes

agraboso mentioned this pull request Aug 11, 2016

Handling a CustomBusinessDay in time-based .rolling() #13969

Closed

sinhrks added Enhancement Resample resample method labels Aug 11, 2016

sinhrks reviewed Aug 11, 2016
View reviewed changes

jreback reviewed Aug 11, 2016
View reviewed changes

jreback added Reshaping Concat, Merge/Join, Stack/Unstack, Explode Difficulty Intermediate and removed Difficulty Intermediate labels Aug 11, 2016

jreback reviewed Dec 20, 2016

View reviewed changes

jreback added this to the 0.20.0 milestone Feb 27, 2017

jreback removed this from the 0.20.0 milestone Mar 23, 2017

jreback closed this Mar 23, 2017

		@@ -377,6 +381,12 @@ class Window(_Window):

		axis : int or string, default 0

		.. versionadded:: 0.19.0

ENH: allow exact matches in time-based .rolling() #13968

ENH: allow exact matches in time-based .rolling() #13968

Conversation

agraboso commented Aug 11, 2016

Choose a reason for hiding this comment

agraboso Aug 11, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

agraboso Aug 11, 2016 • edited Loading

Choose a reason for hiding this comment

chrisaycock commented Aug 11, 2016 • edited Loading

agraboso commented Aug 11, 2016

chrisaycock commented Aug 11, 2016

agraboso commented Aug 11, 2016

codecov-io commented Aug 11, 2016 • edited Loading

Current coverage is 84.65% (diff: 100%)

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sinhrks commented Aug 11, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jreback commented Aug 11, 2016

agraboso commented Aug 11, 2016

agraboso commented Aug 11, 2016

jreback commented Aug 11, 2016

agraboso commented Aug 11, 2016

jorisvandenbossche commented Aug 11, 2016 • edited Loading

jreback commented Aug 11, 2016

jorisvandenbossche commented Aug 11, 2016 • edited Loading

agraboso commented Aug 12, 2016 • edited Loading

agraboso commented Aug 12, 2016

agraboso commented Aug 12, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jreback commented Dec 20, 2016

jreback commented Dec 30, 2016

jreback commented Feb 27, 2017

chrisaycock commented Feb 27, 2017

jreback commented Feb 27, 2017

jreback commented Feb 27, 2017

jreback commented Mar 14, 2017

chrisaycock commented Mar 14, 2017

jreback commented Mar 14, 2017

jreback commented Mar 14, 2017

carlosdanielcsantos commented Mar 15, 2017

jreback commented Mar 15, 2017

carlosdanielcsantos commented Mar 15, 2017 • edited Loading

jreback commented Mar 15, 2017

chrisaycock commented Mar 15, 2017

jreback commented Mar 15, 2017

carlosdanielcsantos commented Mar 17, 2017

jreback commented Mar 17, 2017

agraboso commented Mar 17, 2017

jreback commented Mar 23, 2017

carlosdanielcsantos commented Mar 23, 2017 • edited Loading

jreback commented Mar 23, 2017

carlosdanielcsantos commented Mar 23, 2017

jreback commented Mar 23, 2017

jreback commented Mar 23, 2017

agraboso Aug 11, 2016 •

edited

Loading

agraboso Aug 11, 2016 •

edited

Loading

chrisaycock commented Aug 11, 2016 •

edited

Loading

codecov-io commented Aug 11, 2016 •

edited

Loading

sinhrks commented Aug 11, 2016 •

edited

Loading

jorisvandenbossche commented Aug 11, 2016 •

edited

Loading

jorisvandenbossche commented Aug 11, 2016 •

edited

Loading

agraboso commented Aug 12, 2016 •

edited

Loading

carlosdanielcsantos commented Mar 15, 2017 •

edited

Loading

carlosdanielcsantos commented Mar 23, 2017 •

edited

Loading