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

ERR: Most consistent error handling when passing win_type='freq' in rolling #15969

Closed
TomAugspurger opened this issue Apr 10, 2017 · 8 comments · Fixed by #38641
Closed

ERR: Most consistent error handling when passing win_type='freq' in rolling #15969

TomAugspurger opened this issue Apr 10, 2017 · 8 comments · Fixed by #38641
Labels
Datetime Datetime data dtype Error Reporting Incorrect or improved errors from pandas Window rolling, ewma, expanding
Milestone

Comments

@TomAugspurger
Copy link
Contributor

TomAugspurger commented Apr 10, 2017

Working on dask/dask#2190 (df.rolling('5s') for dask), and I think these should all be equivalent.

In [26]: import pandas as pd
In [27]: import numpy as np
In [31]: from pandas.tseries.frequencies import to_offset
In [28]: s = pd.Series(range(10), index=pd.date_range('2017', freq='s', periods=10))
>>> s.rolling('2s')  # Case 1: correct
>>> s.rolling(window=2000000000, min_periods=1, win_type='freq')  # Case 2
>>> s.rolling(window=to_offset('2s'), min_periods=1, win_type='freq')  # Case 3
>>> s.rolling(window=pd.Timedelta('2s'), min_periods=1, win_type='freq')  # Same as 3

I don't think there are any parsing ambiguities.

Currently we have

# Case 1
In [33]: s.rolling('2s')
Out[33]: Rolling [window=2000000000,min_periods=1,center=False,win_type=freq,axis=0]
# Case 2
In [35]: s.rolling(window=2000000000, min_periods=1, win_type='freq')
---------------------------------------------------------------------------
ValueError                                Traceback (most recent call last)
<ipython-input-2-a24a29dff0ab> in <module>()
----> 1 s.rolling(window=2000000000, min_periods=1, win_type='freq')

/Users/taugspurger/.virtualenvs/dask-dev/lib/python3.6/site-packages/pandas/core/generic.py in rolling(self, window, min_periods, freq, center, win_type, on, axis)
   5502                                    min_periods=min_periods, freq=freq,
   5503                                    center=center, win_type=win_type,
-> 5504                                    on=on, axis=axis)
   5505
   5506         cls.rolling = rolling

/Users/taugspurger/.virtualenvs/dask-dev/lib/python3.6/site-packages/pandas/core/window.py in rolling(obj, win_type, **kwds)
   1795
   1796     if win_type is not None:
-> 1797         return Window(obj, win_type=win_type, **kwds)
   1798
   1799     return Rolling(obj, **kwds)

/Users/taugspurger/.virtualenvs/dask-dev/lib/python3.6/site-packages/pandas/core/window.py in __init__(self, obj, window, min_periods, freq, center, win_type, axis, on, **kwargs)
     76         self.win_type = win_type
     77         self.axis = obj._get_axis_number(axis) if axis is not None else None
---> 78         self.validate()
     79
     80     @property

/Users/taugspurger/.virtualenvs/dask-dev/lib/python3.6/site-packages/pandas/core/window.py in validate(self)
    505                 raise ValueError('Invalid win_type {0}'.format(self.win_type))
    506             if getattr(sig, self.win_type, None) is None:
--> 507                 raise ValueError('Invalid win_type {0}'.format(self.win_type))
    508         else:
    509             raise ValueError('Invalid window {0}'.format(window))

ValueError: Invalid win_type freq
# Case 3
In [36]: s.rolling(window=to_offset('2s'), min_periods=1, win_type='freq')
---------------------------------------------------------------------------
ValueError                                Traceback (most recent call last)
<ipython-input-36-93e14a8d05f6> in <module>()
----> 1 s.rolling(window=to_offset('2s'), min_periods=1, win_type='freq')

/Users/taugspurger/Envs/dask-dev/lib/python3.6/site-packages/pandas/core/generic.py in rolling(self, window, min_periods, freq, center, win_type, on, axis)
   5502                                    min_periods=min_periods, freq=freq,
   5503                                    center=center, win_type=win_type,
-> 5504                                    on=on, axis=axis)
   5505
   5506         cls.rolling = rolling

/Users/taugspurger/Envs/dask-dev/lib/python3.6/site-packages/pandas/core/window.py in rolling(obj, win_type, **kwds)
   1795
   1796     if win_type is not None:
-> 1797         return Window(obj, win_type=win_type, **kwds)
   1798
   1799     return Rolling(obj, **kwds)

/Users/taugspurger/Envs/dask-dev/lib/python3.6/site-packages/pandas/core/window.py in __init__(self, obj, window, min_periods, freq, center, win_type, axis, on, **kwargs)
     76         self.win_type = win_type
     77         self.axis = obj._get_axis_number(axis) if axis is not None else None
---> 78         self.validate()
     79
     80     @property

/Users/taugspurger/Envs/dask-dev/lib/python3.6/site-packages/pandas/core/window.py in validate(self)
    507                 raise ValueError('Invalid win_type {0}'.format(self.win_type))
    508         else:
--> 509             raise ValueError('Invalid window {0}'.format(window))
    510
    511     def _prep_window(self, **kwargs):

ValueError: Invalid window <2 * Seconds>
@TomAugspurger TomAugspurger changed the title Rolling with win_type=freq and DataOffset API: Rolling with win_type=freq and DateOffset or Timedelta Apr 10, 2017
@chris-b1
Copy link
Contributor

In the spirit of #15836, I'd be inclined to disallow case 2, although certainly should raise a better error message.

@TomAugspurger
Copy link
Contributor Author

TomAugspurger commented Apr 10, 2017

In the spirit of #15836, I'd be inclined to disallow case 2

Hmm, yeah I probably agree.

although certainly should raise a better error message.

I've updated the original post to have the traceback with scipy installed. Now it's Invalid win_type freq, so yeah I better error message would be needed to prevent confusion :)

@jreback
Copy link
Contributor

jreback commented Apr 10, 2017

this is a dupe of #15584

and case 2 should not be supported.

@jreback jreback closed this as completed Apr 10, 2017
@jreback jreback added the Duplicate Report Duplicate issue or pull request label Apr 10, 2017
@jreback
Copy link
Contributor

jreback commented Apr 10, 2017

actually these all work (except case 2), except that specifying win_type='freq' is not allowed (nor should it, this would be just way too confusing, that applies to other arguments).

@jreback jreback reopened this Apr 10, 2017
@jreback
Copy link
Contributor

jreback commented Apr 10, 2017

I'll re-open but what is the justification for directly supporting win_type='freq' AND supporting an offset / convertible argument? that seems way to confusing. win_type is a very specialized argument (and invoked a completely different type of rolling window that is VERY limited - on purpose).

@TomAugspurger
Copy link
Contributor Author

Right, win_type is the issue.

The use-case is making the dask implementation a bit easier. Not super difficult to work around. And I was just surprised that

In [16]: s.rolling('2s')
Out[16]: Rolling [window=2000000000,min_periods=1,center=False,win_type=freq,axis=0]

In [17]: s.rolling('2s', win_type='freq')  # TypeError

went down different code-paths. Not a big deal though.

@jorisvandenbossche jorisvandenbossche removed the Duplicate Report Duplicate issue or pull request label Apr 11, 2017
@jorisvandenbossche
Copy link
Member

Can we change the repr of the rolling object to not show win_type=freq ?
As I think this is just confusing, win_type='freq' isn't documented and also shouldn't work (win_type is for how the values in the window are weighted, so a whole different use case I think)

@TomAugspurger
Copy link
Contributor Author

Yeah, I'm fine with that, rather than allowing a user to specify win_type='freq'

@WillAyd WillAyd added the Window rolling, ewma, expanding label Oct 5, 2018
@mroeschke mroeschke added the Bug label Apr 1, 2020
@mroeschke mroeschke changed the title API: Rolling with win_type=freq and DateOffset or Timedelta DEPR: Passing win_type='freq' in rolling Oct 5, 2020
@mroeschke mroeschke added Deprecate Functionality to remove in pandas and removed API Design Bug labels Oct 5, 2020
@mroeschke mroeschke changed the title DEPR: Passing win_type='freq' in rolling ERR: Most consistent error handling when passing win_type='freq' in rolling Dec 22, 2020
@mroeschke mroeschke added Error Reporting Incorrect or improved errors from pandas and removed Deprecate Functionality to remove in pandas labels Dec 22, 2020
@jreback jreback added this to the 1.3 milestone Dec 22, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Datetime Datetime data dtype Error Reporting Incorrect or improved errors from pandas Window rolling, ewma, expanding
Projects
None yet
Development

Successfully merging a pull request may close this issue.

7 participants