Skip to content

BUG: rolling window iterator with on should replace with the window index with the on column #40373

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

Closed
2 of 3 tasks
joshlk opened this issue Mar 11, 2021 · 7 comments · Fixed by #41405
Closed
2 of 3 tasks
Labels
Bug Window rolling, ewma, expanding
Milestone

Comments

@joshlk
Copy link

joshlk commented Mar 11, 2021

  • I have checked that this issue has not already been reported.
  • I have confirmed this bug exists on the latest version of pandas.
  • (optional) I have confirmed this bug exists on the master branch of pandas.

Note: Please read this guide detailing how to provide the necessary information for us to reproduce your bug.

Code Sample, a copy-pastable example

df = pd.DataFrame({'A': range(10), 'B': range(10,0,-1)}).reset_index()
for group in df.rolling(5, on='index'):
    print(len(group))

Prints:

1
2
3
4
5
5
5
5
5
5

Problem description

With a rolling window all groups should be always the same size (5 in the above example) and so the first 4 groups should be skipped. The first groups are an expanding window which then turn into a rolling window.

Additionally, the API is a bit unexpected when you compare to a for idx, group in df.groupby('col') operation. With a groupby each element also provides the index, while rolling it just provides the grouped data and removes the grouping index column (i.e. the column index does not exist in group).

Expected Output

5
5
5
5
5
5

Even better it would also include the index so that:

for idx, group in df.rolling(5, on='index'):
    print(len(group))

Would work

Output of pd.show_versions()

INSTALLED VERSIONS

commit : db08276
python : 3.8.5.final.0
python-bits : 64
OS : Linux
OS-release : 4.15.0-1100-azure
Version : #111~16.04.1-Ubuntu SMP Thu Nov 19 06:49:21 UTC 2020
machine : x86_64
processor : x86_64
byteorder : little
LC_ALL : en_US.UTF-8
LANG : en_US.UTF-8
LOCALE : en_US.UTF-8

pandas : 1.1.3
numpy : 1.19.2
pytz : 2020.1
dateutil : 2.8.1
pip : 20.2.4
setuptools : 50.3.1.post20201107
Cython : 0.29.17
pytest : 6.1.1
hypothesis : None
sphinx : 3.2.1
blosc : None
feather : None
xlsxwriter : 1.3.7
lxml.etree : 4.6.1
html5lib : 1.1
pymysql : None
psycopg2 : None
jinja2 : 2.11.2
IPython : 7.19.0
pandas_datareader: None
bs4 : 4.9.3
bottleneck : 1.3.2
fsspec : 0.8.3
fastparquet : None
gcsfs : None
matplotlib : 3.3.2
numexpr : 2.7.1
odfpy : None
openpyxl : 3.0.5
pandas_gbq : None
pyarrow : 3.0.0
pytables : None
pyxlsb : None
s3fs : None
scipy : 1.5.2
sqlalchemy : 1.3.20
tables : 3.6.1
tabulate : None
xarray : None
xlrd : 1.2.0
xlwt : 1.3.0
numba : 0.51.2

@joshlk joshlk added Bug Needs Triage Issue that has not been reviewed by a pandas team member labels Mar 11, 2021
@mroeschke
Copy link
Member

From the beginning example in the windowing user guide, this is the expected behavior as windowing results are designed to return a result that's the same result as the original DataFrame/Series: https://pandas.pydata.org/docs/user_guide/window.html#windowing-operations

As for iterating while specifying on, I suppose the on column should be brought along with the window.

@mroeschke mroeschke changed the title BUG: rolling window iterator provides an expanding window BUG: rolling window iterator with on should replace with the window index with the on column Mar 11, 2021
@mroeschke mroeschke added Window rolling, ewma, expanding and removed Needs Triage Issue that has not been reviewed by a pandas team member labels Mar 11, 2021
@joshlk
Copy link
Author

joshlk commented Mar 11, 2021

Sorry I missed that example in the docs. Looking at the example, the first group in s.rolling(window=2) is one row with the value 0. However the first value of rolling(window=2).sum() is NaN. How is this consistent?

@mroeschke
Copy link
Member

The first result value is dictated by the min_periods argument: https://pandas.pydata.org/pandas-docs/stable/reference/api/pandas.DataFrame.rolling.html

@joshlk
Copy link
Author

joshlk commented Mar 11, 2021

It says:

min_periods will default to the size of the window

So shouldn't min_periods == 5 in the above example?

@mroeschke
Copy link
Member

Correct

In [1]: df = pd.DataFrame({'A': range(10), 'B': range(10,0,-1)}).reset_index()

In [2]: df.rolling(5, on='index').sum()
Out[2]:
      A     B
0   NaN   NaN
1   NaN   NaN
2   NaN   NaN
3   NaN   NaN
4  10.0  40.0
5  15.0  35.0
6  20.0  30.0
7  25.0  25.0
8  30.0  20.0
9  35.0  15.0

In [3]: df.rolling(5, on='index', min_periods=5).sum()
Out[3]:
      A     B
0   NaN   NaN
1   NaN   NaN
2   NaN   NaN
3   NaN   NaN
4  10.0  40.0
5  15.0  35.0
6  20.0  30.0
7  25.0  25.0
8  30.0  20.0
9  35.0  15.0

@joshlk
Copy link
Author

joshlk commented Mar 12, 2021

The above makes sense. What I believe is inconsistent is how it works when used in a for loop:

for df in df.rolling(5, on='index'):
    print(df['A'].sum())

Outputs:

0
1
3
6
10
15
20
25
30
35

This is because the first element of the iterator is:

>>> list(df.rolling(5, on='index'))[0]
    A   B
0   0  10

What would make more sense is either for that element to not exist or for the first element to include NaNs so that a .sum() would produce a NaN.

Do you agree that df.rolling(5, on='index').sum() is acting differently to for df in df.rolling(5, on='index')?

@mroeschke
Copy link
Member

I believe Rolling.__iter__ was designed to just slice the original DataFrame/Series to produce windows and not make any assumptions of what should be done with the objects.

@jreback jreback added this to the 1.3 milestone May 10, 2021
@mroeschke mroeschke removed this from the 1.3 milestone May 11, 2021
@jreback jreback added this to the 1.3 milestone May 21, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Bug Window rolling, ewma, expanding
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants