8000 PERF/BUG: use masked algo in groupby cummin and cummax by mzeitlin11 · Pull Request #40651 · pandas-dev/pandas · GitHub
[go: up one dir, main page]
Skip to content

PERF/BUG: use masked algo in groupby cummin and cummax #40651

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 44 commits into from
Apr 21, 2021
Merged

PERF/BUG: use masked algo in groupby cummin and cummax #40651

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
44 commits
Select commit Hold shift + click to select a range
7cd4dc3
wip
mzeitlin11 Mar 26, 2021
91984dc
wip
mzeitlin11 Mar 26, 2021
b371cc5
wip
mzeitlin11 Mar 26, 2021
69cce96
wip
mzeitlin11 Mar 26, 2021
dd7f324
wip
mzeitlin11 Mar 26, 2021
64680d4
wip
mzeitlin11 Mar 26, 2021
be16f65
wip
mzeitlin11 Mar 26, 2021
9442846
wip
mzeitlin11 Mar 26, 2021
f089175
wip
mzeitlin11 Mar 26, 2021
31409f8
wip
mzeitlin11 Mar 26, 2021
0c05f74
wip
mzeitlin11 Mar 26, 2021
18dcc94
Merge remote-tracking branch 'origin/master' into perf/masked_cummin/max
mzeitlin11 Mar 26, 2021
5c60a1f
wip
mzeitlin11 Mar 26, 2021
f0c27ce
PERF: use masked algo in groupby cummin and cummax
mzeitlin11 Mar 27, 2021
2fa80ad
Avoid mask copy
mzeitlin11 Mar 27, 2021
280c7e5
Update whatsnew
mzeitlin11 Mar 27, 2021
dca28cf
Merge remote-tracking branch 'origin/master' into perf/masked_cummin/max
mzeitlin11 Apr 1, 2021
7e2fbe0
Merge fixup
mzeitlin11 Apr 1, 2021
0ebb97a
Follow transpose
mzeitlin11 Apr 1, 2021
0009dfd
Compute mask usage inside algo
mzeitlin11 Apr 1, 2021
6663832
try optional
mzeitlin11 Apr 1, 2021
8247f82
WIP
mzeitlin11 Apr 1, 2021
71e1c4f
Use more contiguity
mzeitlin11 Apr 1, 2021
c6cf9ee
Shrink benchmark
mzeitlin11 Apr 1, 2021
02768ec
Merge remote-tracking branch 'origin/master' into perf/masked_cummin/max
mzeitlin11 Apr 1, 2021
836175b
Merge remote-tracking branch 'origin/master' into perf/masked_cummin/max
mzeitlin11 Apr 2, 2021
293dc6e
Revert unrelated
mzeitlin11 Apr 2, 2021
478c6c9
Merge remote-tracking branch 'origin/master' into perf/masked_cummin/max
mzeitlin11 Apr 6, 2021
fa45a9a
Merge remote-tracking branch 'origin/master' into perf/masked_cummin/max
mzeitlin11 Apr 8, 2021
1632b81
Merge remote-tracking branch 'origin/master' into perf/masked_cummin/max
mzeitlin11 Apr 12, 2021
1bb344e
Remove merge conflict relic
mzeitlin11 Apr 12, 2021
97d9eea
Update doc/source/whatsnew/v1.3.0.rst
mzeitlin11 Apr 13, 2021
892a92a
Update doc/source/whatsnew/v1.3.0.rst
mzeitlin11 Apr 13, 2021
a239a68
Update pandas/core/groupby/ops.py
mzeitlin11 Apr 13, 2021
f98ca35
Merge remote-tracking branch 'origin' into perf/masked_cummin/max
mzeitlin11 Apr 13, 2021
e7ed12f
Merge branch 'perf/masked_cummin/max' of github.com:/mzeitlin11/panda…
mzeitlin11 Apr 13, 2021
a1422ba
Address comments
mzeitlin11 Apr 13, 2021
482a209
Change random generation style
mzeitlin11 Apr 13, 2021
4e7404d
Merge remote-tracking branch 'origin' into perf/masked_cummin/max
mzeitlin11 Apr 18, 2021
251c02a
Use conditional instead of partial
mzeitlin11 Apr 18, 2021
3de7e5e
Remove ensure_int_or_float
mzeitlin11 Apr 18, 2021
237f86f
Remove unnecessary condition
mzeitlin11 Apr 18, 2021
a1b0c04
Merge remote-tracking branch 'origin' into perf/masked_cummin/max
mzeitlin11 Apr 19, 2021
5e1dac4
Merge remote-tracking branch 'origin' into perf/masked_cummin/max
mzeitlin11 Apr 20, 2021
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
Prev Previous commit
Next Next commit
PERF: use masked algo in groupby cummin and cummax
  • Loading branch information
@mzeitlin11
mzeitlin11 committed Mar 27, 2021
commit f0c27ce09a1158391015569128d0fafacd85a06d
32 changes: 32 additions & 0 deletions asv_bench/benchmarks/groupby.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -395,8 +395,40 @@ class GroupByMethods:
params = [
["int", "float", "object", "datetime"],
[
"all",
"any",
"bfill",
"count",
"cumcount",
"cummax",
"cummin",
"cumprod",
"cumsum",
"describe",
"ffill",
"first",
"head",
"last",
"mad",
"max",
"min",
"median",
"mean",
"nunique",
"pct_change",
"prod",
"quantile",
"rank",
"sem",
"shift",
"size",
"skew",
"std",
"sum",
"tail",
"unique",
"value_counts",
"var",
],
["direct", "transformation"],
]
Expand Down
2 changes: 1 addition & 1 deletion pandas/_libs/groupby.pyx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1299,7 +1299,7 @@ def group_cummin_max(groupby_t[:, ::1] out,
bint val_is_nan

N, K = (<object>values).shape
accum = np.empty((ngroups, K), dtype=np.asarray(values).dtype, order="C")
accum = np.empty((ngroups, K), dtype=np.asarray(values).dtype)
if groupby_t is int64_t:
accum[:] = -_int64_max if compute_max else _int64_max
elif groupby_t is uint64_t:
Expand Down
13 changes: 5 additions & 8 deletions pandas/core/groupby/ops.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -484,7 +484,6 @@ def _get_cython_func_and_vals(
-------
func : callable
values : np.ndarray
needs_mask : True if a mask must be passed
"""
try:
func = _get_cython_function(kind, how, values.dtype, is_numeric)
Expand All @@ -500,8 +499,7 @@ def _get_cython_func_and_vals(
func = _get_cython_function(kind, how, values.dtype, is_numeric)
else:
raise
needs_mask = how in _CYTHON_FUNCTIONS["needs_mask"]
return func, values, needs_mask
return func, values

@final
def _disallow_invalid_ops(
Expand Down Expand Up @@ -656,8 +654,9 @@ def _cython_operation(
# if not raise NotImplementedError
self._disallow_invalid_ops(dtype, how, is_numeric)

func_uses_mask = cython_function_uses_mask(how)
if is_extension_array_dtype(dtype):
if isinstance(values, BaseMaskedArray) and cython_function_uses_mask(how):
if isinstance(values, BaseMaskedArray) and func_uses_mask:
return self._masked_ea_wrap_cython_operation(
kind, values, how, axis, min_count, **kwargs
Copy link
Contributor

Choose a reason for hiding this comment

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

i really don't understand all of this code duplication. this is adding huge complexity. pls reduce it.

Copy link
Member
@jorisvandenbossche jorisvandenbossche Apr 13, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

Jeff, did you actually read the previous responses to your similar comment? (https://github.com/pandas-dev/pandas/pull/40651/files#r603319910) Can you then please answer there to the concrete reasons given.

Copy link
Contributor

Choose a reason for hiding this comment

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

yes and its a terrible pattern.

Copy link
Contributor

Choose a reason for hiding this comment

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

this duplication of code is ridiculous. We have a VERY large codebase. Having this kind of separate logic is amazingling confusing & is humungous tech debt. This is heavily used code and needs to be carefully modified.

Copy link
Member Author
@mzeitlin11 mzeitlin11 Apr 13, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

I understand the concern about adding code complexity - my thinking was that if the goal is for nullable types to become the default in pandas, then direct support makes sense. And in that case, nullable types would need to be special-cased somewhere, and I think the separate function is cleaner than interleaving in _ea_wrap_cython_operation.

If direct support for nullable dtypes is not desired, we can just close this. If it is, I'll keep trying to think of ways to achieve this without adding more code, but any suggestions there would be welcome!

Copy link
Member
@jorisvandenbossche jorisvandenbossche Apr 13, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

Proper support for nullable dtypes is certainly desired (how to add it exactly can of course be discussed), so thanks a lot @mzeitlin11 for your efforts here.

AFAIK, it's correct we need some special casing for it somewhere (that's the whole point of this PR is to add special handling for it).
Where exactly to put this special casing can of course be discussed, but to me the separate helper method instead of interleaving it in _ea_wrap_cython_operation seems good (I don't think that interleaving it into the existing _ea_wrap_cython_operation would result in fewer added lines of code (and would be harder to read)).

@jreback please try to stay constructive (eg answer to our arguments or provide concrete suggestions on where you would put it / how you would do it differently) and please mind your language (there is no need to call the approach taken by a contributor "terrible").

Copy link
Member

Choose a reason for hiding this comment

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

  1. I agree with @jorisvandenbossche on phrasing concerns. Even the best of us slip up here from time to time.

  2. if the goal is for nullable types to become the default in pandas

This decision has not been made.

  1. I think the separate function is cleaner than interleaving in _ea_wrap_cython_operation.

Agreed.

  1. My preferred dispatch logic would look something like:
def _cython_operation(...)
    if is_ea_dtype(...):
       return self. _ea_wrap_cython_operation(...)
    [status quo]

def _ea_wrap_cython_operation(...):
    if should_use_mask(...):
        return self._masked_ea_wrap_cython_operation(...)
    [status quo]

as Joris correctly pointed out, that is not viable ATM. I think a lot of this dispatch logic eventually belongs in WrappedCythonOp (which i've been vaguely planning on doing next time there aren't any open PRs touching this code), at which point we can reconsider flattening this

  1. My other preferred dispatch logic would not be in this file at all, but be implemented as a method on the EA subclass. I'm really uncomfortable with this code depending on MaskedArray implementation details, seeing as how there has been discussion of swapping them out for something arrow-based.

Copy link
Member Author

Choose a reason for hiding this comment

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

@jbrockmendel if you plan further refactoring of this code, I'm happy to just mothball this pr for now. The real benefit won't come in until more groupby algos allow a mask on this path anyway, so not worth adding now if it's just going to cause more pain in future refactoring.

I also like the idea of approach 5 instead of this - could start looking into that if you think it's a promising direction.

Copy link
Member

Choose a reason for hiding this comment

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

if you plan further refactoring of this code, I'm happy to just mothball this pr for now.

From today's call, I think the plan is to move forward with this first.

I also like the idea of approach 5 instead of this - could start looking into that if you think it's a promising direction.

Long-term I think this is the right way to go to get the general case right, so I'd encourage you if you're interested in trying to implement this on the EA- separate PR(s).

)
Expand Down Expand Up @@ -706,11 +705,9 @@ def _cython_operation(
)
out_shape = (self.ngroups,) + values.shape[1:]

func, values, needs_mask = self._get_cython_func_and_vals(
kind, how, values, is_numeric
)
func, values = self._get_cython_func_and_vals(kind, how, values, is_numeric)
use_mask = mask is not None
if needs_mask:
if func_uses_mask:
if mask is None:
mask = np.zeros_like(values, dtype=np.uint8, order="C")

Expand Down
0