-
-
Notifications
You must be signed in to change notification settings - Fork 18.6k
PERF: masked ops for reductions (sum) #30982
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 9 commits
9795f35
7fb1f88
cd26920
28cd331
6298fbd
735a741
6df454f
5eb48d6
3ef1331
d2230fd
19ac821
2776436
68b4dc2
18d5bfa
457efb1
4df858f
f5120db
76c5149
476f768
b2162dc
d4746f5
d9c2cbf
f8705c2
1a43e10
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 | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
@@ -6,6 +6,7 @@ | |||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
import numpy as np | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
from pandas._libs import lib, missing as libmissing | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
from pandas.compat.numpy import _np_version_under1p17 | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
def kleene_or( | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
@@ -176,3 +177,55 @@ def kleene_and( | |||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
def raise_for_nan(value, method): | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
if lib.is_float(value) and np.isnan(value): | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
raise ValueError(f"Cannot perform logical '{method}' with floating NaN") | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
def sum( | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
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. this entire file is in a really weird place. we have pandas/core/nanops.py which this is duplicating lots of things, and the eventual home of pandas/core/array_algos/ where I think this should live (once merged with nanops). I think this PR needs to move things in the right place, rather than having duplicated code. A pre-cursor PR to move things around would be ok too. Having duplicated code is a huge drag and needs to be patched sooner rather than later. I believe we had this discussion originally when this location was selected (pre 1.0.0) |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
values: np.ndarray, mask: np.ndarray, skipna: bool, min_count: int = 0, | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
): | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
""" | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
Sum for 1D masked array. | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
Parameters | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
---------- | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
values : np.ndarray | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
Numpy array with the values (can be of any dtype that support the | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
operation). | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
mask : np.ndarray | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
Boolean numpy array (True values indicate missing values). | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
skipna : bool, default True | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
Whether to skip NA. | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
min_count : int, default 0 | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
The required number of valid values to perform the operation. If fewer than | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
``min_count`` non-NA values are present the result will be NA. | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
""" | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
if not skipna: | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
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. most of the code here is not 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. Yes, that's possible. I mainly want to implement first a single one, so we can review this / agree on the principles. Afterwards there can be more PRs implementing the other reductions, and then we should indeed see what's the best way to do this to avoid duplication. |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
if mask.any(): | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
return libmissing.NA | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
else: | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
if _below_min_count(values, None, min_count): | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
return libmissing.NA | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
return np.sum(values) | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
else: | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
if _below_min_count(values, mask, min_count): | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
return libmissing.NA | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
if _np_version_under1p17: | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
return np.sum(values[~mask]) | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
else: | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
return np.sum(values, where=~mask) | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
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. out of curiosity, is this more efficient than the version used in older numpy? 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. Good question. With the array in my top post it gives only a slight boost (a 5-8% speed-up), but it varies quite a bit (I could also have it slower with more np.nan values).
Although the difference gets smaller when there are more missing values (eg with 50% missing values instead of 10% in the above, it becomes 25 vs 32ms). But I assume there is also a memory benefit (avoiding the temporary array), although I am not fully familiar with the inner workings of this in numpy. |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
def _below_min_count(values, mask, min_count): | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
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. we already do this in the nanops sum yes? can u de duplicate 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. Yes, it's here: Lines 1238 to 1271 in 37a7006
But I would prefer not to deduplicate, as the one in nanops is mixed with other logic / more complex because it needs to handle more dimensions. 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. disagree this is more code to maintain which keeps happening with ios 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, what's "with ios" ? |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
""" | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
Check for the `min_count` keyword. Returns True if below `min_count` (when | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
pd.NA should be returned from the reduction). | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
""" | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
if min_count > 0: | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
if mask is None: | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
# no missing values, only check size | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
non_nulls = values.size | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
else: | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
non_nulls = mask.size - mask.sum() | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
if non_nulls < min_count: | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
return True | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
return False |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -533,13 +533,14 @@ def test_sum_inf(self): | |
res = nanops.nansum(arr, axis=1) | ||
assert np.isinf(res).all() | ||
|
||
@pytest.mark.parametrize("dtype", ["float64", "Int64", "boolean"]) | ||
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. should object be included here? 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. maybe 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. Added object dtype |
||
@pytest.mark.parametrize("use_bottleneck", [True, False]) | ||
@pytest.mark.parametrize("method, unit", [("sum", 0.0), ("prod", 1.0)]) | ||
def test_empty(self, method, unit, use_bottleneck): | ||
def test_empty(self, method, unit, use_bottleneck, dtype): | ||
with pd.option_context("use_bottleneck", use_bottleneck): | ||
# GH#9422 / GH#18921 | ||
# Entirely empty | ||
s = Series([], dtype=object) | ||
s = Series([], dtype=dtype) | ||
# NA by default | ||
result = getattr(s, method)() | ||
assert result == unit | ||
|
@@ -562,8 +563,14 @@ def test_empty(self, method, unit, use_bottleneck): | |
result = getattr(s, method)(skipna=True, min_count=1) | ||
assert pd.isna(result) | ||
|
||
result = getattr(s, method)(skipna=False, min_count=0) | ||
assert result == unit | ||
|
||
result = getattr(s, method)(skipna=False, min_count=1) | ||
assert pd.isna(result) | ||
|
||
# All-NA | ||
s = Series([np.nan]) | ||
s = Series([np.nan], dtype=dtype) | ||
# NA by default | ||
result = getattr(s, method)() | ||
assert result == unit | ||
|
@@ -587,7 +594,7 @@ def test_empty(self, method, unit, use_bottleneck): | |
assert pd.isna(result) | ||
|
||
# Mix of valid, empty | ||
s = Series([np.nan, 1]) | ||
s = Series([np.nan, 1], dtype=dtype) | ||
# Default | ||
result = getattr(s, method)() | ||
assert result == 1.0 | ||
|
@@ -606,22 +613,22 @@ def test_empty(self, method, unit, use_bottleneck): | |
result = getattr(s, method)(skipna=True, min_count=0) | ||
assert result == 1.0 | ||
|
||
result = getattr(s, method)(skipna=True, min_count=1) | ||
assert result == 1.0 | ||
|
||
# GH#844 (changed in GH#9422) | ||
df = DataFrame(np.empty((10, 0))) | ||
df = DataFrame(np.empty((10, 0)), dtype=dtype) | ||
assert (getattr(df, method)(1) == unit).all() | ||
|
||
s = pd.Series([1]) | ||
s = pd.Series([1], dtype=dtype) | ||
result = getattr(s, method)(min_count=2) | ||
assert pd.isna(result) | ||
|
||
s = pd.Series([np.nan]) | ||
result = getattr(s, method)(skipna=False, min_count=2) | ||
assert pd.isna(result) | ||
|
||
s = pd.Series([np.nan], dtype=dtype) | ||
result = getattr(s, method)(min_count=2) | ||
assert pd.isna(result) | ||
|
||
s = pd.Series([np.nan, 1]) | ||
s = pd.Series([np.nan, 1], dtype=dtype) | ||
result = getattr(s, method)(min_count=2) | ||
assert pd.isna(result) | ||
|
||
|
Uh oh!
There was an error while loading. Please reload this page.