ENH: use same value counts for roll_var to remove floating point artifacts
State: closed

ENH: use same value counts for roll_var to remove floating point artifacts
Please refer to https://github.com/pandas-dev/pandas/issues/42064#issuecomment-1072934337 for the explaination of this new method.
- [ ] closes #42064 (Replace xxxx with the Github issue number)
- [x] [Tests added and passed](https://pandas.pydata.org/pandas-docs/dev/development/contributing_codebase.html#writing-tests) if fixing a bug or adding a new feature
- [x] All [code checks passed](https://pandas.pydata.org/pandas-docs/dev/development/contributing_codebase.html#pre-commit).
- [x] Added an entry in the latest `doc/source/whatsnew/vX.X.X.rst` file if fixing a bug or adding a new feature.


Labels:
name: Algos
description: Non-arithmetic algos: value_counts, factorize, sorting, isin, clip, shift, diff
name: Window
description: rolling, ewma, expanding

Comments:
Looks like there's a network issue in the test.
Isn't there logic that needs to be added to `remove_var` too? Essentially if we need to track if each window has all unique, non-nan values, then this sliding window algorithm needs to account for values moving out of the window?  
> Isn't there logic that needs to be added to `remove_var` too? Essentially if we need to track if each window has all unique, non-nan values, then this sliding window algorithm needs to account for values moving out of the window?

Here's my thoughts:

Suppose we have x number of same values, then the window enters into some nans. Now `same_value_count` is still x. Then we enter a window with valid numbers.

If the new number is still the same, `same_value_count` will continue to increment by 1. Though it's actutally larger than num of same values in current window, we're safe to set result as zero. If the new number differs,  `same_value_count` will be reset to 1.

So in summary, I think it's OK that `same_value_count` is not updated in `remove_var`, because `same_value_count` basically records the num of same value in the entire series.

I will write some tests in the upcoming days, but I'm not sure where I should put them. Do I need to add some in `pandas/tests/window/test_rolling.py`?

If only doc test is needed, do I just have to write some examples in rolling.var, std 's docs (in fact I already update the old examples in the docs)?
Gotcha. Not sure if there's a condition where `remove_var` is called in an iteration while `add_var` is not which could also maybe mess up the counting(?), but if not then the resetting should work. 

`pandas/tests/window/test_rolling.py` is fine for the tests. I don't think we specifically need more doc examples for this.

It would be good to have an example like

`data = pd.Series([0, 0, 0, 1, 2, 3, 3, 3, 2, 1]).rolling(3).var()`

But the `0, 1, 2, 3` are numbers where the floating point artifacts used to show up 
> Gotcha. Not sure if there's a condition where `remove_var` is called in an iteration while `add_var` is not which could also maybe mess up the counting(?), but if not then the resetting should work.

The case where only `remove_var` is called happens when it enters a series of NaN or it goes to the end. For that first case, I've added corresponding tests.

Basically, the counting needn't care about removed values, as long as we know how many `x` have appeared when we see it, and the count is correct, then it's OK even if we count `x` from the beginning of the array.

And we only need to make sure every time we see a new number in `add_var`, the count updates correctly.
The test failed in `py38_32bit`. Do I need to do anything about it? Seems like a bizarre error
Not so familiar with git. After I clicked "sync fork" in pycharm, my branch was added commits from others which may be the cause of these test failures(?) 😓

My last version failed in `py38_32bit` and I can't figure out why. @mroeschke Would you tell me what I should do next?
I would recommend following the contributing guide to set up git correctly (not sure how Pycharm decided to do this): https://pandas.pydata.org/docs/development/contributing.html#working-with-the-code

Namely you probably need to merge the main branch into your branch

```
git remote add upstream https://github.com/pandas-dev/pandas.git
git checkout roll_var_remove_floating_point_artifacts
git fetch upstream
git merge upstream/main
```
@mroeschke I reset my branch to the earliest state and merge with main and then force-pushed it. It really works.
The tests all passed expect for the doc build. But that seems to be a bug in main?
@mroeschke looks like `Py_ssize_t` can't pass 32bit test
@mroeschke 

> Could you also modify `test_rolling_var_floating_artifact_precision` and `test_rolling_std_small_values` and remove the `atol` and `rtol` parameter (since they shouldn't be needed anymore) and add a `# GH <PR number>` in the test?

If we remove the tol params (1.0e-15), the default tol (1.0e-5 & 1.0e-8) is looser. Should I enable `check_exact`? But I'm afraid we can't pass 32-bit test in this way:

Cuz here I can't use 4 / 3 (=1.3333333333333333), but 1.3333333333333335:

https://github.com/pandas-dev/pandas/blob/672798ec7b5565366be24aa4f57f6374f3a5e0fd/pandas/tests/window/test_rolling.py#L1518-L1523


Since the new algo only changes values which are supposed to be real zeros, I recommend add a equal-zero assertion, just as I did in `test_rolling_var_same_value_count_logic`

>  I recommend add a equal-zero assertion, just as I did in test_rolling_var_same_value_count_logic

Okay, agreed that would be good to add in these tests.
@mroeschke 
I'm thinking if I should update the algo for other rolling functions as well. 
Today I encountered a bug caused by rolling mean:
```python
pd.Series([ -0.022556,  0.046154, -0.051471, -0.015504,
       -0.03937 ,  0.008197, -0.02439 , -0.05    , -0.052632, -0.046296,
        0.      ,  0.      ,  0.      ,  0.      ,  0.      ,  0.      ,
        0.      ,  0.      ]).rolling(5).mean()
```
That outputs:
```
0              NaN
1              NaN
2              NaN
3              NaN
4    -1.654940e-02
5    -1.039880e-02
6    -2.450760e-02
7    -2.421340e-02
8    -3.163900e-02
9    -3.302420e-02
10   -3.466360e-02
11   -2.978560e-02
12   -1.978560e-02
13   -9.259200e-03
14    1.387779e-18
15    1.387779e-18
16    1.387779e-18
17    1.387779e-18
dtype: float64
```
This is really annoying. Previously I thought these floating artifacts would not happen in `mean` as it's implemented with Kahan summation. But now I'd say maybe we need to add same-value counts for `mean` \ `sum`, too?


>  I'm thinking if I should update the algo for other rolling functions as well.

Sure, I think this would be better in a follow-up PR
@mroeschke 
> Sure, I think this would be better in a follow-up PR

Should I open seperate PRs for different functions or merge them in a bigger PR? I'm looking into `sum`, `mean`, `std`, `var`, `skew`, `kurt`. 

As for `skew` & `kurt`, they currently output NaN for small divisors:
https://github.com/pandas-dev/pandas/blob/672798ec7b5565366be24aa4f57f6374f3a5e0fd/pandas/_libs/window/aggregations.pyx#L418-L434

```python
r = pd.Series([ -0.022556,  0.046154, -0.051471, -0.015504,
       -0.03937 ,  0.008197, -0.02439 , -0.05    , -0.052632, -0.046296,
        0.      ,  0.      ,  0.      ,  0.      ,  0.      ,  0.      ,
        0.      ,  0.      ]).rolling(5)
r.skew()
```
```
0          NaN
1          NaN
2          NaN
3          NaN
4     1.482404
5     0.655211
6     0.444657
7     0.516469
8     1.289529
9     1.387878
10    1.182903
11    0.577833
12   -0.639457
13   -2.236068
14         NaN
15         NaN
16         NaN
17         NaN
dtype: float64
```
the last 4 number is NaN instead of 0.
`r.kurt` also outputs NaN.

do not merge to anything bigger

wait for review and merge here
The former tests here require "all rolling kurt for all equal values should return Nan" (which referenced a wrong issue id?#18804)
With the new impl, the default result of same values is 0 for skew and -3 for kurt.
Possiblly related: #30993

https://github.com/pandas-dev/pandas/blob/b3b5e2a46ac160eab600a7760bb5953c0e541bf9/pandas/tests/window/test_rolling_skew_kurt.py#L221-L230
> This looked pretty good, just needs to resolve the merge conflict

I put these changes in __Other enhancements__ section, not sure if it's right.
@auderson could you merge main one more time?
Awesome thanks @auderson (failure unrelated)! 
Nice👍