Center rolling window for time offset
State: closed

- [x] closes #20012
- [x] tests added / passed
- [x] passes `black pandas`
- [ ] passes `git diff upstream/master -u -- "*.py" | flake8 --diff`
- [x] 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 https://github.com/pandas-dev/pandas/pull/36097

Labels:
name: Window
description: rolling, ewma, expanding

Comments:
take
Unfortunately I am currently sunk in work, but I will come back to implement the tests in the next week! Thanks for all the feedback so far. 👍 
@jreback we implemented testing of the new "center" to two already existing tests now. Is that what you had in mind?
Hey @mroeschke, 

We've encountered some unexpected behavior with the test "test_closed_fixed" (in pandas/tests/window/test_rolling.py), and before we make any substantial changes to the related functions (which are somewhat out of context of this PR, in our opinion), then we wanted to get your feedback.

As you can see in the screenshot below, when using the regular "rolling" functionality (i.e. without a time-index) and `center=True` we are not getting the result we would expect. It seems like the results are shifted by one index, and that there is a NaN value at the zeroth index. You can see this clearly when invoking a call to rolling with a `window=2`, `center=True`,  `closed="neither"`,  `min_periods=1`, and aggregating with `.mean()`. Then this should exactly reproduce the original Series. As you can see this works as expected when using a time-index (which we have just implemented), but does not appear to work otherwise. 

![image](https://user-images.githubusercontent.com/11252091/106453596-d7dfd600-6489-11eb-9240-f42a1e9d6f4d.png)

Maybe I am missing something simple here? Or is this indicative of a bug in the non-time-index rolling functionalities? If the latter, this is probably something that should be tackled in a separate PR?

@sevberg you bring up an interesting edge case. I think the fundamental question you're asking is "When centering a window with 2 points (or any even number of values), should the left edge or the right edge be considered the "center" "

I suppose there is a valid case for picking either, but my best answer is that it might be "pandas convention" that the left edge was chosen (which is the behavior that you're seeing above). I would recommend for the PR trying to match what the fixed window behavior does and if you disagree with the existing behavior you can open up a separate issue to discuss.

For reference, here's how the window edges for fixed window lengths are calculated: https://github.com/pandas-dev/pandas/blob/9c33e5e4f4c5dd6941da866234e506dae7d9f1e0/pandas/core/window/indexers.py#L81

> For reference, here's how the window edges for fixed window lengths are calculated:
> 
> https://github.com/pandas-dev/pandas/blob/9c33e5e4f4c5dd6941da866234e506dae7d9f1e0/pandas/core/window/indexers.py#L81

Thanks a lot for pointing that out, @mroeschke. I completely understand your point about how this is an arbitrary decision when working in "index space", and why it's currently implemented in this way :).  As you suggested, I've tried to align the datetimelike-window behavior with the regular index-window but, after trying to think it through to ensure a general solution, I've ran into a few logical inconstancies which may make it impossible. In the end, I think it may be both easier and more "correct" to have the rolling functionality behave slightly differently in the "index-window" versus the "datetimelike-window" contexts when using a centered window. Hopefully I can convince you of this as well :)

The differences fundamentally stems from the consequence of working in "index space", where the limitation you described above applies, versus that of "continuous space" such as for a date-time-index or float-valued-index where it does not. In the latter case, it is always possible to define a window (lets call this "W") centered on some value ("X") by calculating X-W/2 to X+W/2, and this window will always be valid regardless if W is an even number (or even a whole number). If I wanted to make the centered-datetimelike-rolling behavior the same as the "index space" behavior, then I would also have to decide on what the smallest possible time-unit is ("U"). Afterwards I would be able to use this to determine if the given window is "effectively" even or odd, and then finally shift the window in the even case (via X-W/2+U/2 to X+W/2+U/2 ). But since we're working in "continuous space", then there is no such smallest time-unit. So considering that adding this shifting is not necessary (since the definition of the window is valid without it), would complicate the behavior of the function, and would be based on an arbitrary assumption, then I think it would be better not to include it at all.

Hopefully you see this the same way I do. If so, I've already added a new test to specifically test rolling functions for centered datetimelike windows.

Either way, I'm looking forward to your feedback! 
Also, I've identified a small bug (a real one, this time :p) in the way the `sem` function is calculated. In short, in the "pandas/core/nanops" functions you can see that the `ddof` variable is not subtracted from the array count, where as in the rolling version of the same function, it is included. I assume that the rolling version of this function should match the "global" one, yes? If so, this is already corrected :)
@sevberg I agree with your explanation in theory but

> If I wanted to make the centered-datetimelike-rolling behavior the same as the "index space" behavior, then I would also have to decide on what the smallest possible time-unit is ("U")

"U" in this case is 1 nanosecond as the cython code that determines the window bounds for rolling datetimes deals with int64 representations of the times (epoch nanoseconds).
hello again @mroeschke, you're totally right to point that out. But I guess +/- one nanosecond doesn't make much of a difference for the [test in question](https://github.com/pandas-dev/pandas/blob/8e2e98baa62de4629ed3240abb66be9cc1c3ead5/pandas/tests/window/test_rolling.py#L124-L135) where the time-step is in days. So do I understand correctly that it is alright to move forward with testing centered datetimelike-windows separately from index-widows?
> pls add a whatsnew note in Enh for 1.3. Also see if any docs need updating in window.rst

Hello @jreback, I've added a, enhancement note in whatsnew (v1.3), but I did not see something in window.rst which obviously needed attention. Did you have something specific in mind?
@sevberg true given that test in question, having a different result for time based rolling windows is okay.

Could you also add test case though where that +/- 1 nanosecond does matter?
Hello @luholgit! Thanks for updating this PR. We checked the lines you've touched for [PEP 8](https://www.python.org/dev/peps/pep-0008) issues, and found:







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

##### Comment last updated at 2021-04-09 12:15:28 UTC
thanks @luholgit very nice!
Big thanks to you, @jreback and @mroeschke! I've learned a lot while working on this. :)