-
-
Notifications
You must be signed in to change notification settings - Fork 18.4k
Rolling window endpoints inclusion #15795
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
Changes from all commits
34f1309
da034bf
2cf6804
5eaf3b4
0e8e65c
ec4bbc7
306b9f7
8bd336a
90dfb0c
c18a31b
037b84e
568c12f
aad97dc
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -56,11 +56,12 @@ | |
|
||
class _Window(PandasObject, SelectionMixin): | ||
_attributes = ['window', 'min_periods', 'freq', 'center', 'win_type', | ||
'axis', 'on'] | ||
'axis', 'on', 'closed'] | ||
exclusions = set() | ||
|
||
def __init__(self, obj, window=None, min_periods=None, freq=None, | ||
center=False, win_type=None, axis=0, on=None, **kwargs): | ||
center=False, win_type=None, axis=0, on=None, closed=None, | ||
**kwargs): | ||
|
||
if freq is not None: | ||
warnings.warn("The freq kw is deprecated and will be removed in a " | ||
|
@@ -71,6 +72,7 @@ def __init__(self, obj, window=None, min_periods=None, freq=None, | |
self.blocks = [] | ||
self.obj = obj | ||
self.on = on | ||
self.closed = closed | ||
self.window = window | ||
self.min_periods = min_periods | ||
self.freq = freq | ||
|
@@ -101,6 +103,10 @@ def validate(self): | |
if self.min_periods is not None and not \ | ||
is_integer(self.min_periods): | ||
raise ValueError("min_periods must be an integer") | ||
if self.closed is not None and self.closed not in \ | ||
['right', 'both', 'left', 'neither']: | ||
raise ValueError("closed must be 'right', 'left', 'both' or " | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. is there validation for a fixed window when closed is NOT both? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Again, there is a validation for when closed is not None in Rolling.validate() |
||
"'neither'") | ||
|
||
def _convert_freq(self, how=None): | ||
""" resample according to the how, return a new object """ | ||
|
@@ -374,8 +380,14 @@ class Window(_Window): | |
on : string, optional | ||
For a DataFrame, column on which to calculate | ||
the rolling window, rather than the index | ||
closed : string, default None | ||
Make the interval closed on the 'right', 'left', 'both' or | ||
'neither' endpoints. | ||
For offset-based windows, it defaults to 'right'. | ||
For fixed windows, defaults to 'both'. Remaining cases not implemented | ||
for fixed windows. | ||
|
||
.. versionadded:: 0.19.0 | ||
.. versionadded:: 0.20.0 | ||
|
||
axis : int or string, default 0 | ||
|
||
|
@@ -717,12 +729,12 @@ def _apply(self, func, name=None, window=None, center=None, | |
raise ValueError("we do not support this function " | ||
"in _window.{0}".format(func)) | ||
|
||
def func(arg, window, min_periods=None): | ||
def func(arg, window, min_periods=None, closed=None): | ||
minp = check_minp(min_periods, window) | ||
# ensure we are only rolling on floats | ||
arg = _ensure_float64(arg) | ||
return cfunc(arg, | ||
window, minp, indexi, **kwargs) | ||
window, minp, indexi, closed, **kwargs) | ||
|
||
# calculation function | ||
if center: | ||
|
@@ -731,11 +743,13 @@ def func(arg, window, min_periods=None): | |
|
||
def calc(x): | ||
return func(np.concatenate((x, additional_nans)), | ||
window, min_periods=self.min_periods) | ||
window, min_periods=self.min_periods, | ||
closed=self.closed) | ||
else: | ||
|
||
def calc(x): | ||
return func(x, window, min_periods=self.min_periods) | ||
return func(x, window, min_periods=self.min_periods, | ||
closed=self.closed) | ||
|
||
with np.errstate(all='ignore'): | ||
if values.ndim > 1: | ||
|
@@ -768,7 +782,8 @@ def count(self): | |
for b in blocks: | ||
result = b.notnull().astype(int) | ||
result = self._constructor(result, window=window, min_periods=0, | ||
center=self.center).sum() | ||
center=self.center, | ||
closed=self.closed).sum() | ||
results.append(result) | ||
|
||
return self._wrap_results(results, blocks, obj) | ||
|
@@ -789,11 +804,10 @@ def apply(self, func, args=(), kwargs={}): | |
offset = _offset(window, self.center) | ||
index, indexi = self._get_index() | ||
|
||
def f(arg, window, min_periods): | ||
def f(arg, window, min_periods, closed): | ||
minp = _use_window(min_periods, window) | ||
return _window.roll_generic(arg, window, minp, indexi, | ||
offset, func, args, | ||
kwargs) | ||
return _window.roll_generic(arg, window, minp, indexi, closed, | ||
offset, func, args, kwargs) | ||
|
||
return self._apply(f, func, args=args, kwargs=kwargs, | ||
center=False) | ||
|
@@ -864,7 +878,7 @@ def std(self, ddof=1, *args, **kwargs): | |
def f(arg, *args, **kwargs): | ||
minp = _require_min_periods(1)(self.min_periods, window) | ||
return _zsqrt(_window.roll_var(arg, window, minp, indexi, | ||
ddof)) | ||
self.closed, ddof)) | ||
|
||
return self._apply(f, 'std', check_minp=_require_min_periods(1), | ||
ddof=ddof, **kwargs) | ||
|
@@ -911,7 +925,7 @@ def quantile(self, quantile, **kwargs): | |
def f(arg, *args, **kwargs): | ||
minp = _use_window(self.min_periods, window) | ||
return _window.roll_quantile(arg, window, minp, indexi, | ||
quantile) | ||
self.closed, quantile) | ||
|
||
return self._apply(f, 'quantile', quantile=quantile, | ||
**kwargs) | ||
|
@@ -1044,6 +1058,10 @@ def validate(self): | |
elif self.window < 0: | ||
raise ValueError("window must be non-negative") | ||
|
||
if not self.is_datetimelike and self.closed is not None: | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @jreback here we enforce that |
||
raise ValueError("closed only implemented for datetimelike " | ||
"and offset based windows") | ||
|
||
def _validate_monotonic(self): | ||
""" validate on is monotonic """ | ||
if not self._on.is_monotonic: | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
expand this a touch to say that only
closed='both'
is accepted for fixed windows.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am only accepting closed=None for fixed windows, as previously requested by you
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
that's what i mean just make sure the docs are clear on that ; e.g. None -> 'both' for fixed (and assert that as well )