Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

BUG: rolling_max with center=True and min_periods=1 returns incorrect results #6795

Closed
apuapaquola opened this issue Apr 4, 2014 · 4 comments · Fixed by #30222
Closed

BUG: rolling_max with center=True and min_periods=1 returns incorrect results #6795

apuapaquola opened this issue Apr 4, 2014 · 4 comments · Fixed by #30222
Labels
good first issue Needs Tests Unit test(s) needed to prevent regressions Window rolling, ewma, expanding
Milestone

Comments

@apuapaquola
Copy link

rolling_max with center=True and min_periods=1 returns NaN's

Python 3.3.5 (default, Mar 10 2014, 03:21:31) 
[GCC 4.8.2 20140206 (prerelease)] on linux
Type "help", "copyright", "credits" or "license" for more information.
>>> import pandas as pd
>>> df=pd.DataFrame([0,1,2,1,0], columns=['a']

### This is OK
>>> pd.rolling_max(df['a'], window=3, center=True)
0   NaN
1     2
2     2
3     2
4   NaN
dtype: float64


### This should return [1, 2, 2, 2, 1]
>>> pd.rolling_max(df['a'], window=3, center=True, min_periods=1)
0     1
1     2
2     2
3     2
4   NaN
dtype: float64

>>> pd.show_versions()

INSTALLED VERSIONS
------------------
commit: None
python: 3.3.5.final.0
python-bits: 64
OS: Linux
OS-release: 3.13.6-1-ARCH
machine: x86_64
processor: 
byteorder: little
LC_ALL: None
LANG: en_US.UTF-8

pandas: 0.13.1
Cython: 0.20.1
numpy: 1.8.1
scipy: 0.13.3
statsmodels: 0.5.0
IPython: 1.2.1
sphinx: 1.2.2
patsy: None
scikits.timeseries: None
dateutil: 2.2
pytz: 2014.1
bottleneck: None
tables: None
numexpr: None
matplotlib: 1.3.1
openpyxl: None
xlrd: None
xlwt: None
xlsxwriter: None
sqlalchemy: None
lxml: None
bs4: None
html5lib: None
bq: None
apiclient: None

@jreback jreback added this to the 0.15.0 milestone Apr 4, 2014
@apuapaquola
Copy link
Author

Looking at the function _rolling_moment and _center_window in pandas/stats/moments.py, it looks like the moment is calculated the same way as with center=False, and then shifted by half size of window. Calling the shift function introduces NaN's.

def _center_window(rs, window, axis):
    ...
    offset = int((window - 1) / 2.)
    if isinstance(rs, (Series, DataFrame, Panel)):
        rs = rs.shift(-offset, axis=axis)
    ...

@TomAugspurger
Copy link
Contributor

Yeah. It looks like the Cython function are completely unaware about the center argument. I'm trying to come up with a fix that keeps the separation.

I suppose we could always calculate the function for the tail and then throw away the one's we don't need. So in this case, with min_periods=1, the result of func() would be array([ 0., 1., 2., 2., 2., 1, 1, 0, 0, 0]). The last 5 are the tail that aren't calculated now. Then we apply the labels and the shift to center. Does that sound like it would work? We may even be able to calculate fewer extra items...

@hayd
Copy link
Contributor

hayd commented Sep 2, 2014

came up here (I think) http://stackoverflow.com/q/25628587/1240268

@jreback jreback modified the milestones: 0.16.0, Next Major Release Mar 6, 2015
@mroeschke
Copy link
Member

Looks fixed on master. Could use a test.

In [65]: df['a'].rolling(3, center=True, min_periods=1).max()
Out[65]:
0    1.0
1    2.0
2    2.0
3    2.0
4    1.0
Name: a, dtype: float64

@mroeschke mroeschke added good first issue Needs Tests Unit test(s) needed to prevent regressions Window rolling, ewma, expanding and removed Algos Non-arithmetic algos: value_counts, factorize, sorting, isin, clip, shift, diff Bug labels Sep 29, 2019
@jreback jreback modified the milestones: Contributions Welcome, 1.0 Dec 12, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
good first issue Needs Tests Unit test(s) needed to prevent regressions Window rolling, ewma, expanding
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants