[BUG]: Implement Kahan summation for rolling().mean() to avoid numerical issues #36348

phofl · 2020-09-13T22:50:51Z

closes BUG: Mean of zeros turn out to be non-zero #36031
closes Rolling mean on a time-based period with one data point does not return the exact mean #32761
closes rolling_mean returns small negative value on Series with non-negative values #10319
xref Bug when computing rolling_mean with extreme value #11645 (fixes the issue expect the 0, will add an additional pr about the doc)
closes PERF/COMPAT: use kahan summation in .rolling(..).sum() #13254
tests added / passed
passes black pandas
passes git diff upstream/master -u -- "*.py" | flake8 --diff
whatsnew entry

I implemented the Kahan summation as suggested by @jreback. I used the variable names from https://en.wikipedia.org/wiki/Kahan_summation_algorithm. If there exists a name convention am not aware of, I am happy to rename the variables.

� Conflicts: � doc/source/whatsnew/v1.2.0.rst � pandas/core/arrays/datetimelike.py

jreback

i believe this is shared code with mean; so pls test that as well

phofl · 2020-09-14T02:34:10Z

Done

mroeschke · 2020-09-14T02:58:21Z

I dont think this hits resample.mean. I think @jreback might have meant just rolling.mean.

Could you do the same for rolling.sum? #13254

phofl · 2020-09-14T08:22:52Z

@mroeschke
Yeah you are right. I don't know what I have seen that made me believe resample.mean() would use the same function. Thanks.

rolling.sum() uses another Cython function. That was not fixed with my implementation. I would be happy to implement Kahan summation there too. Should I open another PR or mix it in here?

jreback · 2020-09-14T11:53:07Z

i believe this is shared code with mean; so pls test that as well

sorry I meant rolling.sum

yeah ideally we could share code, though these are simple enough and we don't want to introduce functions that it may be just easier to duplicate what is needed.

ok on this PR or followon to do rolling.sum

jreback

for this PR or more likely kahan sum, I think there are a couple of issues on the tracker. if you'd have a look can link them all (and use examples from them).

pandas/_libs/window/aggregations.pyx

jreback · 2020-09-14T11:54:01Z

pandas/_libs/window/aggregations.pyx


    # Not NaN
    if notnan(val):
        nobs[0] = nobs[0] + 1
-        sum_x[0] = sum_x[0] + val
+        y = val - c[0]


add a comment that using kahan summation

I already added a comment in the function header. Is this sufficient?

pandas/tests/window/test_rolling.py

phofl · 2020-09-14T17:53:07Z

@jreback Implemented Kahan summation for rolling().sum(). I also added tests for the issues I've found.

Functions for add_mean, remove_mean, add_sum and remove_sum are quite similar now. We could share this code part for these functions.

doc/source/whatsnew/v1.2.0.rst

pandas/tests/window/test_rolling.py

phofl · 2020-09-14T18:35:23Z

Will look into Test failures

phofl · 2020-09-14T20:16:12Z

Tests should be fixed now. Somehow mixed plus and minus up

mroeschke

One more comment otherwise LGTM

mroeschke · 2020-09-15T01:13:01Z

pandas/tests/window/test_rolling.py

+    result = (
+        df.resample("1s").ffill().rolling("3s", closed="left", min_periods=3).mean()
+    )
+    assert result.values[-1] == 0.0


Could we compare the entire DataFrame result for this test as well?

jreback

ok looks good, ex @mroeschke comments.

can you run all of the rolling asv's and report the results. I would expect a small slowdown.

phofl · 2020-09-15T10:21:29Z

@jreback

I ran asv for rolling. Is the output within the range you expected?

      before           after         ratio
     [1b2f1f47]       [c70c91cd]
     <master>         <36031>   
+     1.48±0.02ms       2.64±0.6ms     1.78  rolling.ForwardWindowMethods.time_rolling('Series', 1000, 'int', 'sum')
+     1.77±0.03ms       2.73±0.6ms     1.54  rolling.VariableWindowMethods.time_rolling('Series', '1d', 'float', 'sum')
+        890±10μs      1.28±0.03ms     1.43  rolling.ExpandingMethods.time_expanding('Series', 'float', 'sum')
+     1.69±0.02ms       2.37±0.7ms     1.41  rolling.ForwardWindowMethods.time_rolling('Series', 1000, 'int', 'mean')
+     1.85±0.02ms       2.50±0.2ms     1.35  rolling.ForwardWindowMethods.time_rolling('DataFrame', 1000, 'int', 'sum')
+     1.00±0.03ms      1.32±0.02ms     1.32  rolling.ExpandingMethods.time_expanding('Series', 'float', 'mean')
+     1.26±0.02ms      1.65±0.04ms     1.31  rolling.ExpandingMethods.time_expanding('DataFrame', 'float', 'sum')
+     3.52±0.06ms         4.54±1ms     1.29  rolling.ExpandingMethods.time_expanding('Series', 'float', 'count')
+     1.42±0.03ms       1.81±0.5ms     1.27  rolling.ForwardWindowMethods.time_rolling('Series', 1000, 'float', 'sum')
+     1.35±0.02ms      1.68±0.02ms     1.24  rolling.ExpandingMethods.time_expanding('DataFrame', 'float', 'mean')
+     1.59±0.05ms      1.92±0.06ms     1.21  rolling.Methods.time_rolling('DataFrame', 1000, 'float', 'sum')
+     1.66±0.05ms      1.93±0.05ms     1.16  rolling.Methods.time_rolling('DataFrame', 1000, 'float', 'mean')
+     1.71±0.02ms       1.98±0.2ms     1.16  rolling.VariableWindowMethods.time_rolling('Series', '1h', 'float', 'sum')
+     4.33±0.04ms       4.78±0.3ms     1.10  rolling.Pairwise.time_pairwise(10, 'cov', False)

SOME BENCHMARKS HAVE CHANGED SIGNIFICANTLY.
PERFORMANCE DECREASED.

jreback · 2020-09-15T22:21:05Z

thanks @phofl very nice!

yeah this is a sligth perf hit, but worth the tradeoff

…cal issues (pandas-dev#36348)

phofl added 3 commits September 14, 2020 00:46

Implement Kahan summation for rolling mean

f12c078

Merge branch 'master' of https://github.com/pandas-dev/pandas

46f499a

� Conflicts: � doc/source/whatsnew/v1.2.0.rst � pandas/core/arrays/datetimelike.py

Change variable names

6d67d19

phofl added Window rolling, ewma, expanding Bug labels Sep 14, 2020

jreback requested changes Sep 14, 2020

View reviewed changes

Add test for resample mean

971fb26

Remove unnecessary test

5c8d7cc

jreback requested changes Sep 14, 2020

View reviewed changes

jreback added this to the 1.2 milestone Sep 14, 2020

jreback added Algos Non-arithmetic algos: value_counts, factorize, sorting, isin, clip, shift, diff and removed Bug labels Sep 14, 2020

phofl added 2 commits September 14, 2020 15:01

Rename variable and parametrize test

58cc035

Implement Kahan summation for rolling sum and add tests for issues

a4e3884

mroeschke reviewed Sep 14, 2020

View reviewed changes

doc/source/whatsnew/v1.2.0.rst Outdated Show resolved Hide resolved

mroeschke reviewed Sep 14, 2020

View reviewed changes

pandas/tests/window/test_rolling.py Outdated Show resolved Hide resolved

mroeschke reviewed Sep 14, 2020

View reviewed changes

pandas/tests/window/test_rolling.py Outdated Show resolved Hide resolved

phofl added 2 commits September 14, 2020 22:12

Change tests and fix typo

9cd1ebe

Fix whats new

4c5b351

mroeschke reviewed Sep 15, 2020

View reviewed changes

jreback requested changes Sep 15, 2020

View reviewed changes

Add df to assert equal

c70c91c

mroeschke approved these changes Sep 15, 2020

View reviewed changes

jreback approved these changes Sep 15, 2020

View reviewed changes

jreback merged commit 078f88e into pandas-dev:master Sep 15, 2020

phofl deleted the 36031 branch September 16, 2020 11:59

rhshadrach pushed a commit to rhshadrach/pandas that referenced this pull request Sep 17, 2020

[BUG]: Implement Kahan summation for rolling().mean() to avoid numeri…

45a917a

…cal issues (pandas-dev#36348)

kesmit13 pushed a commit to kesmit13/pandas that referenced this pull request Nov 2, 2020

[BUG]: Implement Kahan summation for rolling().mean() to avoid numeri…

e173701

…cal issues (pandas-dev#36348)

erinov1 mentioned this pull request Oct 26, 2022

rolling method returns different results in Polars and Pandas pola-rs/polars#5325

Open

2 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[BUG]: Implement Kahan summation for rolling().mean() to avoid numerical issues #36348

[BUG]: Implement Kahan summation for rolling().mean() to avoid numerical issues #36348

phofl commented Sep 13, 2020 •

edited

Loading

jreback left a comment

phofl commented Sep 14, 2020

mroeschke commented Sep 14, 2020

phofl commented Sep 14, 2020

jreback commented Sep 14, 2020

jreback left a comment

jreback Sep 14, 2020

phofl Sep 14, 2020

phofl commented Sep 14, 2020 •

edited

Loading

phofl commented Sep 14, 2020

phofl commented Sep 14, 2020

mroeschke left a comment

mroeschke Sep 15, 2020

phofl Sep 15, 2020

jreback left a comment

phofl commented Sep 15, 2020 •

edited

Loading

jreback commented Sep 15, 2020

[BUG]: Implement Kahan summation for rolling().mean() to avoid numerical issues #36348

[BUG]: Implement Kahan summation for rolling().mean() to avoid numerical issues #36348

Conversation

phofl commented Sep 13, 2020 • edited Loading

jreback left a comment

Choose a reason for hiding this comment

phofl commented Sep 14, 2020

mroeschke commented Sep 14, 2020

phofl commented Sep 14, 2020

jreback commented Sep 14, 2020

jreback left a comment

Choose a reason for hiding this comment

jreback Sep 14, 2020

Choose a reason for hiding this comment

phofl Sep 14, 2020

Choose a reason for hiding this comment

phofl commented Sep 14, 2020 • edited Loading

phofl commented Sep 14, 2020

phofl commented Sep 14, 2020

mroeschke left a comment

Choose a reason for hiding this comment

mroeschke Sep 15, 2020

Choose a reason for hiding this comment

phofl Sep 15, 2020

Choose a reason for hiding this comment

jreback left a comment

Choose a reason for hiding this comment

phofl commented Sep 15, 2020 • edited Loading

jreback commented Sep 15, 2020

phofl commented Sep 13, 2020 •

edited

Loading

phofl commented Sep 14, 2020 •

edited

Loading

phofl commented Sep 15, 2020 •

edited

Loading