Skip to content

CLN/TYPE: window aggregation cleanups and typing #30137

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.

Sign up for GitHub

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

Merged
merged 9 commits into from
Dec 10, 2019
Merged

CLN/TYPE: window aggregation cleanups and typing #30137

Show file tree
Hide file tree
Changes from 6 commits
Commits
Show all changes
9 commits
Select commit Hold shift + click to select a range
07c00ed
Move is_monotonic_bounds to the aggregation functions
Dec 7, 2019
ba95bd6
Remove single usage of _check_min in aggregations.pyx
Dec 7, 2019
a73d7b3
remove _check_min
Dec 7, 2019
32015b2
Add some typing
Dec 8, 2019
c79d5c1
Merge remote-tracking branch 'upstream/master' into indexer_cleanups
Dec 8, 2019
e776c67
fix condition
Dec 8, 2019
636166b
Use is_monotonic from _lib.algos
Dec 8, 2019
259b8b4
Add inline helper function
Dec 8, 2019
674894a
Merge remote-tracking branch 'upstream/master' into indexer_cleanups
Dec 9, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
77 changes: 26 additions & 51 deletions pandas/_libs/window/aggregations.pyx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,7 +18,6 @@ cdef extern from "src/headers/cmath" namespace "std":
int signbit(float64_t) nogil
float64_t sqrt(float64_t x) nogil

cimport pandas._libs.util as util
from pandas._libs.util cimport numeric

from pandas._libs.skiplist cimport (
Expand Down Expand Up @@ -48,39 +47,6 @@ cdef inline int int_min(int a, int b): return a if a <= b else b
# periodically revisited to see if it's still true.
#


def _check_minp(win, minp, N, floor=None) -> int:
"""
Parameters
----------
win: int
minp: int or None
N: len of window
floor: int, optional
default 1

Returns
-------
minimum period
"""

if minp is None:
minp = 1
if not util.is_integer_object(minp):
raise ValueError("min_periods must be an integer")
if minp > win:
raise ValueError(f"min_periods (minp) must be <= "
f"window (win)")
elif minp > N:
minp = N + 1
elif minp < 0:
raise ValueError('min_periods must be >= 0')
if floor is None:
floor = 1

return max(minp, floor)


# original C implementation by N. Devillard.
# This code in public domain.
# Function : kth_smallest()
Expand Down Expand Up @@ -183,14 +149,15 @@ cdef inline void remove_sum(float64_t val, int64_t *nobs, float64_t *sum_x) nogi


def roll_sum_variable(ndarray[float64_t] values, ndarray[int64_t] start,
ndarray[int64_t] end, int64_t minp,
bint is_monotonic_bounds=True):
ndarray[int64_t] end, int64_t minp):
cdef:
float64_t sum_x = 0
int64_t s, e
int64_t nobs = 0, i, j, N = len(values)
ndarray[float64_t] output
bint is_monotonic_bounds

is_monotonic_bounds = np.all(np.diff(start) > 0) and np.all(np.diff(end) > 0)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

is this call expensive? we might have something in _libs.algos that short-circuits this check

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good call. Was able to use is_monotonic in _libs.algos

output = np.empty(N, dtype=float)

with nogil:
Expand Down Expand Up @@ -331,14 +298,15 @@ def roll_mean_fixed(ndarray[float64_t] values, ndarray[int64_t] start,


def roll_mean_variable(ndarray[float64_t] values, ndarray[int64_t] start,
ndarray[int64_t] end, int64_t minp,
bint is_monotonic_bounds=True):
ndarray[int64_t] end, int64_t minp):
cdef:
float64_t val, sum_x = 0
int64_t s, e
Py_ssize_t nobs = 0, i, j, neg_ct = 0, N = len(values)
ndarray[float64_t] output
bint is_monotonic_bounds

is_monotonic_bounds = np.all(np.diff(start) > 0) and np.all(np.diff(end) > 0)
output = np.empty(N, dtype=float)

with nogil:
Expand Down Expand Up @@ -493,8 +461,7 @@ def roll_var_fixed(ndarray[float64_t] values, ndarray[int64_t] start,


def roll_var_variable(ndarray[float64_t] values, ndarray[int64_t] start,
ndarray[int64_t] end, int64_t minp, int ddof=1,
bint is_monotonic_bounds=True):
ndarray[int64_t] end, int64_t minp, int ddof=1):
"""
Numerically stable implementation using Welford's method.
"""
Expand All @@ -504,7 +471,9 @@ def roll_var_variable(ndarray[float64_t] values, ndarray[int64_t] start,
int64_t s, e
Py_ssize_t i, j, N = len(values)
ndarray[float64_t] output
bint is_monotonic_bounds

is_monotonic_bounds = np.all(np.diff(start) > 0) and np.all(np.diff(end) > 0)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

make this a function

output = np.empty(N, dtype=float)

with nogil:
Expand Down Expand Up @@ -641,15 +610,16 @@ def roll_skew_fixed(ndarray[float64_t] values, ndarray[int64_t] start,


def roll_skew_variable(ndarray[float64_t] values, ndarray[int64_t] start,
ndarray[int64_t] end, int64_t minp,
bint is_monotonic_bounds=True):
ndarray[int64_t] end, int64_t minp):
cdef:
float64_t val, prev
float64_t x = 0, xx = 0, xxx = 0
int64_t nobs = 0, i, j, N = len(values)
int64_t s, e
ndarray[float64_t] output
bint is_monotonic_bounds

is_monotonic_bounds = np.all(np.diff(start) > 0) and np.all(np.diff(end) > 0)
output = np.empty(N, dtype=float)

with nogil:
Expand Down Expand Up @@ -794,14 +764,15 @@ def roll_kurt_fixed(ndarray[float64_t] values, ndarray[int64_t] start,


def roll_kurt_variable(ndarray[float64_t] values, ndarray[int64_t] start,
ndarray[int64_t] end, int64_t minp,
bint is_monotonic_bounds=True):
ndarray[int64_t] end, int64_t minp):
cdef:
float64_t val, prev
float64_t x = 0, xx = 0, xxx = 0, xxxx = 0
int64_t nobs = 0, i, j, s, e, N = len(values)
ndarray[float64_t] output
bint is_monotonic_bounds

is_monotonic_bounds = np.all(np.diff(start) > 0) and np.all(np.diff(end) > 0)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you make this a cdef function instead of copy?

output = np.empty(N, dtype=float)

with nogil:
Expand Down Expand Up @@ -1030,8 +1001,7 @@ def roll_min_fixed(ndarray[float64_t] values, ndarray[int64_t] start,


def roll_min_variable(ndarray[float64_t] values, ndarray[int64_t] start,
ndarray[int64_t] end, int64_t minp,
bint is_monotonic_bounds=True):
ndarray[int64_t] end, int64_t minp):
"""
Moving max of 1d array of any numeric type along axis=0 ignoring NaNs.

Expand Down Expand Up @@ -1424,10 +1394,7 @@ def roll_generic_variable(object obj,
ndarray[int64_t] start, ndarray[int64_t] end,
int64_t minp,
int offset, object func, bint raw,
object args, object kwargs,
bint is_monotonic_bounds=True):
# is_monotonic_bounds unused since variable algorithm doesn't calculate
# adds/subtracts across windows, but matches other *_variable functions
object args, object kwargs):
cdef:
ndarray[float64_t] output, counts, bufarr
ndarray[float64_t, cast=True] arr
Expand Down Expand Up @@ -1501,7 +1468,15 @@ cdef ndarray[float64_t] _roll_weighted_sum_mean(float64_t[:] values,
if avg:
tot_wgt = np.zeros(in_n, dtype=np.float64)

minp = _check_minp(len(weights), minp, in_n)
if minp > win_n:
raise ValueError(f"min_periods (minp) must be <= "
f"window (win)")
elif minp > in_n:
minp = in_n + 1
elif minp < 0:
raise ValueError('min_periods must be >= 0')

minp = max(minp, 1)

with nogil:
if avg:
Expand Down
11 changes: 2 additions & 9 deletions pandas/core/window/rolling.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -109,7 +109,7 @@ def _on(self):
def is_freq_type(self) -> bool:
return self.win_type == "freq"

def validate(self):
def validate(self) -> None:
if self.center is not None and not is_bool(self.center):
raise ValueError("center must be a boolean")
if self.min_periods is not None and not is_integer(self.min_periods):
Expand Down Expand Up @@ -412,7 +412,7 @@ def _get_roll_func(self, func_name: str) -> Callable:
)
return window_func

def _get_cython_func_type(self, func):
def _get_cython_func_type(self, func: str) -> Callable:
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If possible to add subtypes to Callable typically very helpful

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Unfortunately the arguments to the cython functions for rolling are still not entirely uniform so the argument list can't be typed consistently.

"""
Return a variable or fixed cython function type.

Expand Down Expand Up @@ -517,13 +517,6 @@ def calc(x):
center=self.center,
closed=self.closed,
)
if np.any(np.diff(start) < 0) or np.any(np.diff(end) < 0):
# Our "variable" algorithms assume start/end are
# monotonically increasing. A custom window indexer
# can produce a non monotonic start/end.
return func(
x, start, end, min_periods, is_monotonic_bounds=False
)
return func(x, start, end, min_periods)

else:
Expand Down