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-method is not working properly when called with a timedelta #57549

Open
3 tasks done
malte-fu opened this issue Feb 21, 2024 · 6 comments
Open
3 tasks done
Labels
Bug Window rolling, ewma, expanding

Comments

@malte-fu
Copy link

Pandas version checks

  • I have checked that this issue has not already been reported.

  • I have confirmed this bug exists on the latest version of pandas.

  • I have confirmed this bug exists on the main branch of pandas.

Reproducible Example

df = pd.DataFrame(index=pd.date_range(start="2024-02-01", end="2024-02-21", freq="1d"))
df["A"] = df.index.day.astype(float)
df.iloc[-2, 0] = np.inf
df.iloc[-9, 0] = np.nan
res1 = df.rolling(window=pd.Timedelta("4d")).mean()
res2 = df.rolling(window=4).mean()

Issue Description

When passing a timedelta to the rolling method, entries like nan and inf are being ignored. Also missing values at the beginning of the column are not taken into account for the calculation. In the Reproducible Example both results res1 and res2 should be equal, but they are not.

Expected Behavior

Missing and invalid values should be taken into account properly when passing a TimeDelta.

Installed Versions

INSTALLED VERSIONS

commit : f538741
python : 3.10.2.final.0
python-bits : 64
OS : Windows
OS-release : 10
Version : 10.0.19045
machine : AMD64
processor : AMD64 Family 25 Model 80 Stepping 0, AuthenticAMD
byteorder : little
LC_ALL : None
LANG : None
LOCALE : de_DE.cp1252

pandas : 2.2.0
numpy : 1.26.4
pytz : 2024.1
dateutil : 2.8.2
setuptools : 58.1.0
pip : 21.2.4
Cython : None
pytest : None
hypothesis : None
sphinx : None
blosc : None
feather : None
xlsxwriter : None
lxml.etree : None
html5lib : None
pymysql : None
psycopg2 : None
jinja2 : None
IPython : 8.21.0
pandas_datareader : None
adbc-driver-postgresql: None
adbc-driver-sqlite : None
bs4 : None
bottleneck : None
dataframe-api-compat : None
fastparquet : None
fsspec : None
gcsfs : None
matplotlib : 3.8.2
numba : None
numexpr : None
odfpy : None
openpyxl : 3.1.2
pandas_gbq : None
pyarrow : 15.0.0
pyreadstat : None
python-calamine : None
pyxlsb : None
s3fs : None
scipy : 1.12.0
sqlalchemy : None
tables : None
tabulate : None
xarray : None
xlrd : None
zstandard : None
tzdata : 2023.4
qtpy : None
pyqt5 : None

@malte-fu malte-fu added Bug Needs Triage Issue that has not been reviewed by a pandas team member labels Feb 21, 2024
@rhshadrach
Copy link
Member

Thanks for the report!

In the Reproducible Example both results res1 and res2 should be equal, but they are not.

I don't think this is correct. From the docs:

For a window that is specified by an offset, min_periods will default to 1.
For a window that is specified by an integer, min_periods will default to the size of the window.

Specifying min_periods to be the same for each example, the output then matches.

entries like nan and inf are being ignored.

Can you clarify what you mean here? What is the expected output?

@rhshadrach rhshadrach added Window rolling, ewma, expanding Needs Info Clarification about behavior needed to assess issue and removed Needs Triage Issue that has not been reviewed by a pandas team member labels Feb 27, 2024
@malte-fu
Copy link
Author

Thanks for the fast response.

Indeed you are right and i was wrong. Still i think that for a window specified by an offset the result should be np.nan if the observed window is shorter than the specified offset. (in my example the first 3 days, since the specified offset is 4 days).

Further i would expect the result for the windows inclusing np.inf or np.nan to be np.nan. Expecialle for np.inf since it is a workaround for the related problem (groupby.mean decribed here: #32696)

Thanks for your thoughts on this in advance.

@rhshadrach
Copy link
Member

Still i think that for a window specified by an offset the result should be np.nan if the observed window is shorter than the specified offset. (in my example the first 3 days, since the specified offset is 4 days).

Can it be the case that the offset is not an integer multiple of the data?

Further i would expect the result for the windows inclusing np.inf or np.nan to be np.nan.

For np.nan, we would first need to implement skipna - I'm +1 here. We should probably set up a tracking issue for this for rolling. The one for groupby is here: #15675.

For np.inf, the corresponding result for DataFrame / groupby is in fact inf, so I would think we should get the same here. Further investigations and PRs to fix are welcome!

@rhshadrach rhshadrach removed the Needs Info Clarification about behavior needed to assess issue label Feb 27, 2024
@malte-fu
Copy link
Author

malte-fu commented Mar 1, 2024

Can it be the case that the offset is not an integer multiple of the data?

I am not sure if i understand you right here. What i wanted to say is: The first entries of the observed dataframe should be np.nan until the entries specify an offset >= the offset specified when calling the function. Like in my example the datetimeindex has a frequence of 1 day. The specified window size is 4 days. So the first 3 entries (window size only 3 days but offset is 4 days) is too small to evaluate the specified window.

@rhshadrach
Copy link
Member

rhshadrach commented Mar 1, 2024

And for e.g. df.rolling(window=pd.Timedelta("3.5d")).mean()? Does that result in np.nan when there are only 3 and not 4 days in the window? Or since the 4th data point will never be used (even if it existed, should the result be non-nan?

In any case, I would recommend having this issue focus on the np.inf bug. The behavior with window being non-integral is documented and functioning correctly in accordance with the docs. If you think this behavior should be changed, can you open up a new issue?

@stevenschaerer
Copy link
Contributor

@rhshadrach I briefly looked into why infs are being ignored. The reason is the function _prep_values which contains the lines

# Convert inf to nan for C funcs
inf = np.isinf(values)
if inf.any():
    values = np.where(inf, np.nan, values)

_prep_values is called in what seems like all the window aggregation functions and infs have been replaced by nans at least since the rolling module was created in 2015.

I'd be happy to work on treating infs as infs, but after all this time I'm not sure we can change the default behavior by calling this a bug. What do you think about adding a skipinf parameter with default True? This would be a bit more flexible than something like treat_inf_as_na and implementing skipna.

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

No branches or pull requests

3 participants