ENH: allow exact matches in time-based .rolling()
State: closed

- [x] closes #13965
- [x] tests added / passed
- [x] passes `git diff upstream/master | flake8 --diff`
- [ ] whatsnew entry

TODO: update docs.


Labels:
name: Enhancement
name: Reshaping
description: Concat, Merge/Join, Stack/Unstack, Explode
name: Resample
description: resample method

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

> 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`.

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.

> 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.

## [Current coverage](https://codecov.io/gh/pandas-dev/pandas/pull/13968?src=pr) is 84.65% (diff: 100%)
> Merging [#13968](https://codecov.io/gh/pandas-dev/pandas/pull/13968?src=pr) into [master](https://codecov.io/gh/pandas-dev/pandas/branch/master?src=pr) will increase coverage by **<.01%**

```diff
@@             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](https://codecov.io?src=pr). Last update [3ccb501...62ff604](https://codecov.io/gh/pandas-dev/pandas/compare/3ccb50131b698bccee21780c660a70bed87396d0...62ff60488d2cc1fa5a1e71dae6ac4ce5e3c65ce6?src=pr)
Pls also add tests with `PeriodIndex` and `TimedeltaIndex` (the latter should raise an error)

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`

> we have the convention in `pd.date_range`

Are you speaking in favor of something different from `left_closed`?

@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?

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

@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. 

@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 :-)

only allow right and both then

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

I proposed `closed=True|False` in https://github.com/pydata/pandas/issues/13965#issuecomment-239236751 to keep the keyword name, but `closed='right'|'both'` (without left) is then maybe better to keep the passed values also consistent.

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.

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

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

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

Option: `closed='right'|'both'`, default `'right'`. Please like to vote for this option.

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

@agraboso `TimedeltaIndex` and `PeriodIndex` are other datetime-related index than `DatetimeIndex`. You can find examples in `test_resample.py``
- https://github.com/pydata/pandas/blob/master/pandas/tseries/tests/test_resample.py#L1431

The voting thing didn't quite work... oh, well. I [changed it to `closed='right'|'both'`](https://github.com/pydata/pandas/pull/13968/commits/b0717ac6bf70442ab3db8bb4a234fa02c41b6743) as per @jreback's suggestion.

@sinhrks I [added tests for `TimedeltaIndex`](https://github.com/pydata/pandas/pull/13968/commits/25e75e92271372652485e860e1a44972aa5147a6) (passing: I have no idea why it should raise, since [`Rolling.is_datetimelike`](https://github.com/agraboso/pandas/blob/25e75e92271372652485e860e1a44972aa5147a6/pandas/core/window.py#L1015) returns `True`).

I looked at the `PeriodIndex` case, but even on `master` it seems to be doing the wrong thing.

```
import pandas as pd

df = pd.DataFrame({'B': range(6)},
                  index=pd.period_range('20130101 09:00:00', periods=6, freq='T'))

print(df)

#                   B
# 2013-01-01 09:00  0
# 2013-01-01 09:01  1
# 2013-01-01 09:02  2
# 2013-01-01 09:03  3
# 2013-01-01 09:04  4
# 2013-01-01 09:05  5
```

I would expect `df.rolling('3s').sum()` to return `df.astype(float)`, but I get the following instead.

```
print(df.rolling('3s').sum())

#                      B
# 2013-01-01 09:00   0.0
# 2013-01-01 09:01   1.0
# 2013-01-01 09:02   3.0
# 2013-01-01 09:03   6.0
# 2013-01-01 09:04  10.0
# 2013-01-01 09:05  15.0
```

The following makes me think the logic for `PeriodIndex` is using the period `ordinal` instead of nanoseconds.

```
print(df.rolling('1ns').sum())

#                     B
# 2013-01-01 09:00  0.0
# 2013-01-01 09:01  1.0
# 2013-01-01 09:02  2.0
# 2013-01-01 09:03  3.0
# 2013-01-01 09:04  4.0
# 2013-01-01 09:05  5.0

print(df.rolling('2ns').sum())

#                     B
# 2013-01-01 09:00  0.0
# 2013-01-01 09:01  1.0
# 2013-01-01 09:02  3.0
# 2013-01-01 09:03  5.0
# 2013-01-01 09:04  7.0
# 2013-01-01 09:05  9.0

print(df.rolling('3ns').sum())

#                      B
# 2013-01-01 09:00   0.0
# 2013-01-01 09:01   1.0
# 2013-01-01 09:02   3.0
# 2013-01-01 09:03   6.0
# 2013-01-01 09:04   9.0
# 2013-01-01 09:05  12.0
```

I can't find any tests in `pandas.tests.test_window.TestRollingTS` with `PeriodIndex` or `TimedeltaIndex` (apart from the ones I just added).

@jreback?

@jreback Would it make sense to set `closed='both'` as the default? That seems more intuitive to me since the right side is always closed, though I want to make sure other people agree first. No one is using this function in the wild yet, so we won't be breaking backwards compatibility by changing it at this stage.

> Would it make sense to set closed='both' as the default? That seems more intuitive to me since the right side is always closed

I think the current behaviour (closed='right') is more logical, at least in the one usecase I am now imagining (other use cases can have other logical defaults). For example, if you have a hourly timeseries, and you want to calculate the 8 hour moving mean, the current behaviour actually takes the mean of 8 hours, while with `closed='both'` it would take the mean of 9 hourly values. 

@chrisaycock agree with @jorisvandenbossche its not natural to by-default include BOTH end-points. Here, the 'right' is the included one, so the 'left' should be open (not-included).

can you rebase this, and move whatsnew to 0.20.0
Rebased, need to add whatsnew.
lgtm (slight doc correction). pls add a whatsnew for 0.20.0

cc @chrisaycock 
@agraboso pls add a whatsnew for 0.20.0 (other enhancements)