API: reimplement FixedWindowIndexer.get_window_bounds to fix groupby bug
State: closed

- [x] closes #36040
- [x] tests added / passed
- [x] passes `black pandas`
- [x] passes `git diff upstream/master -u -- "*.py" | flake8 --diff`
- [ ] whatsnew entry

Creating PR to solve [Issue 36040](https://github.com/pandas-dev/pandas/issues/36040).

The old `FixedWindowIndexer.get_window_bounds` function provided unintuitive bounds that went beyond the length of the original array. Additionally, to do a centered rolling operation, it required NaN values to be appended to the end of original array to enable some roundabout way of achieving the centering.

I replaced it with one that seems much simpler and actually creates "fixed" size windows (at least prior to clipping the ends), which the previous function did not.

That being said, I know this PR fails some tests, I'm would appreciate some advice on how best to proceed!

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

Comments:
Hello @justinessert! 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 2020-09-25 02:51:47 UTC
@arw2019 @jreback @mroeschke Thanks for your feedback, I believe that I addressed all of it, please let me know if there's anything else you want to see in this PR.

I know that I still need to add a whatsnew entry, could you please advise what/where I should add?
Can someone help me understand the test failure? It seems to be with the `to_parquet` function, but only on `Windows py38_np18`
> Can someone help me understand the test failure? It seems to be with the `to_parquet` function, but only on `Windows py38_np18`

I think that's unrelated to this patch
@jreback I agree that the skip_offset param is not ideal, but I currently don't see a way to do this without a similar level of complexity. I'll do my best to explain the reasoning here.

The core of the issue is that there are 4 different types of cython functions being used, which all handle creating a centered window differently:
1. **Fixed Cython Funcs:** Fixed cython funcs require appending NaNs to the original array to create a centered window (this is when offset should be non-zero).
2. **Weighted Cython Funcs:** Similar to Fixed, these also require appending NaNs.
3. **Variable Cython Funcs:** Rather than appending NaNs, start/end arrays handle creating a centered window
4. **.apply(lambda...):** These lambda functions take an offset parameter and handle the centering themselves (rather than using start/end or appending NaNs)

Essentially, the `skip_offset` parameter is just trying to track when we need an offset (ie. `fixed/weighted`) and when we don't need an offset (ie. `variable/.apply(lambda...)`). You're right that in most cases we use `is_freq_type` variable to determine whether to use Fixed/Variable. But there are also many cases with alternate logic:
- As suggested above, their are also cases when we use `weighted cython funcs` or `.apply(lambda...)`, which each use separate logic
- Functions applied to groupby.rolling() will never be a `fixed cython func`, regardless of `is_freq_type`
- [The Median function](https://github.com/pandas-dev/pandas/pull/36132/files#diff-0de5c5d9abfcdd141e83701eaaec4358R1554-R1560) always needs `skip_offset=True` (I believe because it falls into the `variable cython func` category
- [The Quantile function](https://github.com/pandas-dev/pandas/pull/36132/files#diff-0de5c5d9abfcdd141e83701eaaec4358R1716-R1741) is the worst, if `quantile` is 0.0 or 1.0 then it uses the min/max functions which could be either fixed or variable (based on `is_freq_type`) but if `quantile` is something else, then we use the `roll_quantile` function, which is always a variable function.

@jreback does the above comment sufficiently describe why the extra param is needed? If not, what further questions can I answer?
@jreback @arw2019 @mroeschke what questions can I answer? What changes are you looking for? Is this PR good to go?
Sorry for the delay @justinessert.

This PR may become a lot simpler if we can push https://github.com/pandas-dev/pandas/pull/36567 through as we will no longer need to pass around a flag that indicates whether we're using a variable or fixed algorithm
Replacing this PR with [37035](https://github.com/pandas-dev/pandas/pull/37035) following @mroeschke's [36567](https://github.com/pandas-dev/pandas/pull/36567)