-
-
Notifications
You must be signed in to change notification settings - Fork 18.5k
PERF: use non-copying path for Groupby.skew #52104
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 15 commits
f909a2d
293b9c0
5823f8b
ea4d54c
f94a1b3
baafbaa
4ca8bed
1f1a268
294a500
9333c78
3069f4c
bc0b67a
04acbf5
c34cdad
7e84d04
78096ed
67d2669
9609c95
703f2aa
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 |
---|---|---|
|
@@ -892,6 +892,94 @@ def group_var( | |
out[i, j] /= (ct - ddof) | ||
|
||
|
||
@cython.wraparound(False) | ||
@cython.boundscheck(False) | ||
@cython.cdivision(True) | ||
def group_skew( | ||
float64_t[:, ::1] out, | ||
int64_t[::1] counts, | ||
ndarray[float64_t, ndim=2] values, | ||
const intp_t[::1] labels, | ||
const uint8_t[:, ::1] mask=None, | ||
uint8_t[:, ::1] result_mask=None, | ||
bint skipna=True, | ||
) -> None: | ||
cdef: | ||
Py_ssize_t i, j, N, K, lab, ngroups = len(counts) | ||
int64_t[:, ::1] nobs | ||
Py_ssize_t len_values = len(values), len_labels = len(labels) | ||
bint isna_entry, uses_mask = mask is not None | ||
float64_t[:, ::1] M1, M2, M3 | ||
float64_t delta, delta_n, term1, val | ||
int64_t n1, n | ||
float64_t ct, m2_15, root_n | ||
|
||
if len_values != len_labels: | ||
raise ValueError("len(index) != len(labels)") | ||
|
||
nobs = np.zeros((<object>out).shape, dtype=np.int64) | ||
|
||
# M1, M2, and M3 correspond to 1st, 2nd, and third Moments | ||
M1 = np.zeros((<object>out).shape, dtype=np.float64) | ||
M2 = np.zeros((<object>out).shape, dtype=np.float64) | ||
M3 = np.zeros((<object>out).shape, dtype=np.float64) | ||
|
||
N, K = (<object>values).shape | ||
|
||
out[:, :] = 0.0 | ||
|
||
with nogil: | ||
for i in range(N): | ||
lab = labels[i] | ||
if lab < 0: | ||
continue | ||
|
||
counts[lab] += 1 | ||
|
||
for j in range(K): | ||
val = values[i, j] | ||
|
||
if uses_mask: | ||
isna_entry = mask[i, j] | ||
else: | ||
isna_entry = _treat_as_na(val, False) | ||
|
||
if not isna_entry: | ||
# Based on RunningSats::Push from | ||
# https://www.johndcook.com/blog/skewness_kurtosis/ | ||
n1 = nobs[lab, j] | ||
n = n1 + 1 | ||
|
||
nobs[lab, j] = n | ||
delta = val - M1[lab, j] | ||
delta_n = delta / n | ||
term1 = delta * delta_n * n1 | ||
|
||
M1[lab, j] += delta_n | ||
M3[lab, j] += term1 * delta_n * (n - 2) - 3 * delta_n * M2[lab, j] | ||
M2[lab, j] += term1 | ||
elif not skipna: | ||
M1[lab, j] = NaN | ||
M2[lab, j] = NaN | ||
M3[lab, j] = NaN | ||
|
||
for i in range(ngroups): | ||
for j in range(K): | ||
ct = <float64_t>nobs[i, j] | ||
if ct < 3: | ||
if result_mask is not None: | ||
result_mask[i, j] = 1 | ||
out[i, j] = NaN | ||
elif M2[i, j] == 0: | ||
out[i, j] = 0 | ||
else: | ||
m2_15 = <float64_t>(M2[i, j] ** 1.5) | ||
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. I'm not familiar with the rules for casting a complex to a float value, but seems potentially dangerous to do this. Is there no explicit way to get the real part from the complex and make sure the imaginary is safely discardable? 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. no idea, and google is no help. cc @seberg thoughts? 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. I don't get it, all of this is already typed as float64, no? So this uses Otherwise, yes 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. Without the cast we got build failures on the npdev build
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. Sorry, I had checked this line, but the problem is only in the line below :). This must have to do with cython 3 would be my guess? It uses the Python power definition which converts to complex, but you probably want a float power anyway. Maybe try: EDIT: Nvm which line... it must be the Cython version only likely... 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. https://cython.readthedocs.io/en/latest/src/userguide/source_files_and_compilation.html#compiler-directives The 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. Looks like 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. Looks like that did it! |
||
root_n = <float64_t>((ct - 1) ** 0.5) | ||
out[i, j] = ( | ||
(ct * root_n / (ct - 2)) * (M3[i, j] / m2_15) | ||
) | ||
|
||
|
||
@cython.wraparound(False) | ||
@cython.boundscheck(False) | ||
def group_mean( | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,27 @@ | ||
import numpy as np | ||
|
||
import pandas as pd | ||
import pandas._testing as tm | ||
|
||
|
||
def test_groupby_skew_equivalence(): | ||
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. Do we do this for the other functions? I like the idea; seems like it would be good to consolidate this with others in a follow up 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. groupby tests are not super well-organized, would be nice to improve someday |
||
# Test that that groupby skew method (which uses libgroupby.group_skew) | ||
# matches the results of operating group-by-group (which uses nanops.nanskew) | ||
nrows = 1000 | ||
ngroups = 3 | ||
ncols = 2 | ||
nan_frac = 0.05 | ||
|
||
arr = np.random.randn(nrows, ncols) | ||
arr[np.random.random(nrows) < nan_frac] = np.nan | ||
|
||
df = pd.DataFrame(arr) | ||
grps = np.random.randint(0, ngroups, size=nrows) | ||
gb = df.groupby(grps) | ||
|
||
result = gb.skew() | ||
|
||
grpwise = [grp.skew().to_frame(i).T for i, grp in gb] | ||
expected = pd.concat(grpwise, axis=0) | ||
expected.index = expected.index.astype(result.index.dtype) # 32bit builds | ||
tm.assert_frame_equal(result, expected) |
Uh oh!
There was an error while loading. Please reload this page.