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

[auderson]

ENH: use same value counts for roll_var to remove floating point artifacts
Please refer to #42064 (comment) for the explaination of this new method.

• closes BUG: rolling std can't handle mixture of (relatively) big and small numbers #42064 (Replace xxxx with the Github issue number)
• Tests added and passed if fixing a bug or adding a new feature
• All code checks passed.
• Added an entry in the latest doc/source/whatsnew/vX.X.X.rst file if fixing a bug or adding a new feature.

[auderson]

Looks like there's a network issue in the test.

[jreback]

we need a battery of tests that exercise this code.

you can use something like the doc examples as tests. they should fail before and pass after.

need to exercise np.inf and np.nan as well (as you are including this in the code)'

pls parameterize over both std and var

[mroeschke]

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?

[auderson]

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.

[auderson]

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

[mroeschke]

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

[auderson]

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.

[auderson]

The test failed in py38_32bit. Do I need to do anything about it? Seems like a bizarre error

[auderson]

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?

[mroeschke]

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

[auderson]

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

[auderson]

@mroeschke looks like Py_ssize_t can't pass 32bit test

[jreback]

do not merge to anything bigger

wait for review and merge here

[auderson]

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

pandas/pandas/tests/window/test_rolling_skew_kurt.py

Lines 221 to 230 in b3b5e2a

	def test_rolling_skew_eq_value_fperr(step):
	# #18804 all rolling skew for all equal values should return Nan
	a = Series([1.1] * 15).rolling(window=10, step=step).skew()
	assert np.isnan(a).all()
	def test_rolling_kurt_eq_value_fperr(step):
	# #18804 all rolling kurt for all equal values should return Nan
	a = Series([1.1] * 15).rolling(window=10, step=step).kurt()
	assert np.isnan(a).all()

[mroeschke]

This looked pretty good, just needs to resolve the merge conflict

[auderson]

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.

[jreback]

can you make an additional test that tests something like [5, 7, 5, 7, 5, 7] e.g. nothing is repeated yet its likey degenerate.

[mroeschke]

LGTM on green

[mroeschke]

@auderson could you merge main one more time?

[jreback]

lgtm @mroeschke merge on green

[mroeschke]

Awesome thanks @auderson (failure unrelated)!

[auderson]

Nice👍