PERF: O(n) speedup in any/all by re-enabling short-circuiting for bool case #25070
Conversation
Codecov Report
@@ Coverage Diff @@
## master #25070 +/- ##
==========================================
- Coverage 92.37% 92.36% -0.01%
==========================================
Files 166 166
Lines 52403 52409 +6
==========================================
+ Hits 48405 48410 +5
- Misses 3998 3999 +1
Continue to review full report at Codecov.
|
Codecov Report
@@ Coverage Diff @@
## master #25070 +/- ##
==========================================
- Coverage 91.97% 91.96% -0.01%
==========================================
Files 175 175
Lines 52368 52378 +10
==========================================
+ Hits 48164 48171 +7
- Misses 4204 4207 +3
Continue to review full report at Codecov.
|
pandas/core/nanops.py
Outdated
| @@ -201,11 +201,13 @@ def _get_fill_value(dtype, fill_value=None, fill_value_typ=None): | |||
|
|
|||
|
|
|||
| def _get_values(values, skipna, fill_value=None, fill_value_typ=None, | |||
| isfinite=False, copy=True, mask=None): | |||
| isfinite=False, copy=True, mask=None, compute_mask=True): | |||
There was a problem hiding this comment.
so rather than adding another argument, we already have the mask, can you simply have this accept mask=True in this case?
There was a problem hiding this comment.
I ended up taking this approach, but instead making mask a positional argument and taking mask = None to be signifying that a mask has been determined to be unnecessary. If another sigil value would be preferred, I think that would be easy to do.
pandas/core/nanops.py
Outdated
| @@ -362,8 +364,12 @@ def nanany(values, axis=None, skipna=True, mask=None): | |||
| >>> nanops.nanany(s) | |||
| False | |||
| """ | |||
| values, mask, dtype, _, _ = _get_values(values, skipna, False, copy=skipna, | |||
| mask=mask) | |||
| if (hasattr(values, 'dtype') and is_bool_dtype(values.dtype) and | |||
There was a problem hiding this comment.
can you do this logic inside _get_values (maybe update the return signature to return skipna as well)
There was a problem hiding this comment.
I'll investigate - this might require modifying some of the other utility functions to play nice.
There was a problem hiding this comment.
This ended up being tricky to do inside _get_values() as mask is sometimes used to compute array dimensions within certain nanops. My solution was to move the logic outside of _get_values, make mask a positional argument, and remove it from the result tuple. This allows for functions that need it for dimension calculations to always compute it while others can do so optionally.
|
Does the implementation become any simpler after Panel is removed? |
|
can you merge master |
qwhelan
force-pushed
the
any_all_fix
branch
5 times, most recently
from
913dd30 to
94fbe52
Compare
|
Apologies for the delay - interviewing processes have gotten in the way here. @jbrockmendel Not especially. @jreback Rebased on master. I've kept my additions in response to your comments as a separate commit for now - will squash if it seems they're going in the right general direction. My PR to fix the underlying performance bug is here numpy/numpy#12988 and pending merge; hopefully it will make it into numpy 1.17 and to a large extent obviate the need for this change. It may make sense to expand this PR to include all integer Additionally, |
pandas/core/nanops.py
Outdated
| isfinite=False, copy=True, mask=None): | ||
| def _maybe_get_mask(values, skipna, mask): | ||
| """ This function will return a mask iff it is necessary. Otherwise, return | ||
| None when a mask is not needed. |
There was a problem hiding this comment.
can you add parameters / returns in doc-string
pandas/core/nanops.py
Outdated
| """ This function will return a mask iff it is necessary. Otherwise, return | ||
| None when a mask is not needed. | ||
| """ | ||
| if (hasattr(values, 'dtype') and is_bool_dtype(values.dtype)): |
There was a problem hiding this comment.
is this first test needed?
There was a problem hiding this comment.
The first test is needed due to a nanall(Panel) test case
There was a problem hiding this comment.
Panel tests should be gone now
pandas/core/nanops.py
Outdated
|
|
||
|
|
||
| def _get_mask(values, mask, skipna=True): | ||
| if mask is None and skipna: |
pandas/core/nanops.py
Outdated
|
|
||
|
|
||
| def _get_values(values, skipna, mask, fill_value=None, fill_value_typ=None, | ||
| copy=True): | ||
| """ utility to get the values view, mask, dtype |
There was a problem hiding this comment.
can you update the doc-string here
pandas/core/nanops.py
Outdated
| # Boolean data cannot contain nulls, so signal via mask being None | ||
| return None | ||
|
|
||
| mask = _get_mask(values, mask, skipna=skipna) |
There was a problem hiding this comment.
why can't you do this inside _get_mask itself? (the test)
There was a problem hiding this comment.
The _get_counts() function relies on mask providing basic shape information even when skipna=False. Unfortunately, creating an empty mask (eg, mask = np.zeros(values.shape)) to propagate this shape info for nanmean/etc negates the speed benefit to nanall()/nanany(), so two separate approaches are necessary.
There was a problem hiding this comment.
can we pass mask=None in those cases? I find this having to call 2 functions much more overhead than before.
pandas/core/nanops.py
Outdated
| """ This function will return a mask iff it is necessary. Otherwise, return | ||
| None when a mask is not needed. | ||
| """ | ||
| if (hasattr(values, 'dtype') and is_bool_dtype(values.dtype)): |
There was a problem hiding this comment.
Panel tests should be gone now
pandas/core/nanops.py
Outdated
| # Boolean data cannot contain nulls, so signal via mask being None | ||
| return None | ||
|
|
||
| mask = _get_mask(values, mask, skipna=skipna) |
There was a problem hiding this comment.
can we pass mask=None in those cases? I find this having to call 2 functions much more overhead than before.
|
can you merge master and update to comments |
| def setup(self, N, dtype): | ||
| self.s = Series([1] * N, dtype=dtype) | ||
|
|
||
| def time_var(self, N, dtype): |
There was a problem hiding this comment.
could make the function another param
There was a problem hiding this comment.
I'll probably remove these prior to merge as they duplicate existing benchmarks.
qwhelan
force-pushed
the
any_all_fix
branch
7 times, most recently
from
3573c21 to
d96b086
Compare
|
I'm going to see if I can simplify further - I was able to remove the "copy=" parameter from That being said, allowing |
pandas/core/nanops.py
Outdated
| ---------- | ||
| values : ndarray | ||
| skipna : bool | ||
| mask : Optional[ndarray[bool]] |
There was a problem hiding this comment.
so rather than typing here, can you type the args themselves
There was a problem hiding this comment.
Done, and also typed the return value. It appears the convention is to duplicate types in the return signature and docstring block; let me know if you'd prefer to keep only one.
There was a problem hiding this comment.
great. cc @WillAyd @jorisvandenbossche need to think about this
but prob ok for now
pandas/core/nanops.py
Outdated
| @@ -200,12 +200,84 @@ def _get_fill_value(dtype, fill_value=None, fill_value_typ=None): | |||
| return tslibs.iNaT | |||
|
|
|||
|
|
|||
| def _maybe_get_mask(values, skipna, mask, invert=False): | |||
There was a problem hiding this comment.
do we really need invert? this is just confusing
There was a problem hiding this comment.
pls type annotate the parameters
There was a problem hiding this comment.
invert was necessary in nanmedian(), but the median calculation is so slow there's minimal impact, so reverting that bit back and removing invert
pandas/core/nanops.py
Outdated
| skipna : bool | ||
| fill_value : Any, optional | ||
| value to fill NaNs with | ||
| fill_value_typ : str, optional |
pandas/core/nanops.py
Outdated
| if skipna and not is_any_int_dtype(values): | ||
| mask = _maybe_get_mask(values, skipna, mask) | ||
|
|
||
| if skipna and not is_any_int_dtype(values) and mask is not None: |
There was a problem hiding this comment.
do you need the int check here? (as the mask will by definition be None if we have ints)
There was a problem hiding this comment.
No. The odd case of Series(dtype='bool').prod() will be slightly faster as a result of it being removed.
pandas/core/nanops.py
Outdated
| if mask is not None: | ||
| null_mask = mask.size - mask.sum() | ||
| else: | ||
| null_mask = functools.reduce(operator.mul, shape) |
There was a problem hiding this comment.
is this line covered in tests? isn't this just np.prod(shape)
There was a problem hiding this comment.
Yeah, this should be getting hit by Series(dtype=int).sum() and the like. I'll confirm once codecov finishes.
pandas/core/nanops.py
Outdated
| @@ -200,12 +200,84 @@ def _get_fill_value(dtype, fill_value=None, fill_value_typ=None): | |||
| return tslibs.iNaT | |||
|
|
|||
|
|
|||
| def _maybe_get_mask(values, skipna, mask, invert=False): | |||
There was a problem hiding this comment.
pls type annotate the parameters
qwhelan
force-pushed
the
any_all_fix
branch
6 times, most recently
from
4f9c8eb to
6ac741b
Compare
|
Updated to reflect comments - the single failing test appears to be unrelated. |
| def setup(self, N, dtype): | ||
| self.s = Series([1] * N, dtype=dtype) | ||
|
|
||
| def time_var(self, N, dtype): |
| @@ -569,7 +664,7 @@ def _get_counts_nanvar(mask, axis, ddof, dtype=float): | |||
| count = np.nan | |||
| d = np.nan | |||
| else: | |||
| mask2 = count <= ddof | |||
| mask2 = count <= ddof # type: np.ndarray | |||
There was a problem hiding this comment.
mypy analysis needed an extra hint for the following line. It deduced that mask2: Union[bool, np.ndarray] and throws an error as bool does not have an any() function.
|
ping |
|
thanks @qwhelan very nice! |
At present, the
.all()and.any()functions scale asO(n), more or less regardless of their exit condition being fulfilled:The root cause of this was identified by
asv findto be #8550, which was released as part ofv0.15.2. Specifically, we callisna()on the entire input prior to dispatching tonp.all/np.any; the latter does short circuit but is insignificant compared toisna().My proposal here is to exempt
np.boolfrom theisna()call on the basis that it is not capable of storingNaNvalues; I'm hesitant to make this case much broader than necessary to accommodate calls likepd.isnull(s).all(). Thehasattr()check is necessary due to aPaneltest case directly invokingnanall(Panel).The good news is that this approach is very effective:
Additionally, this call is not too widely used within the
pandascodebase as most invoke thenumpyequivalent. That being said, we do see some broad speedups when running the entire suite:For the largest
ntested, we see a speedup of~170xin the fast case.git diff upstream/master -u -- "*.py" | flake8 --diff