PERF: groupby with string dtype

[debnathshoham]

• closes PERF: groupby performance regression in 1.2.x #41596
• tests added / passed
• Ensure all linting tests pass, see here for how to run them
• whatsnew entry

In [1]: import numpy as np
   ...:    ...: import pandas as pd
   ...:    ...: pd.__version__
   ...:    ...:
   ...:    ...: cols = list('abcdefghjkl')
   ...:    ...: df = pd.DataFrame(np.random.randint(0, 100, size=(100, len(cols))), columns=cols)
   ...:    ...: df_str = df.astype(str)
   ...:    ...: df_string = df.astype('string')

In [2]: %timeit df_str.groupby('a')[cols[1:]].last()
2.88 ms ± 70.5 µs per loop (mean ± std. dev. of 7 runs, 100 loops each)

In [3]: %timeit df_string.groupby('a')[cols[1:]].last()
3.27 ms ± 12.8 µs per loop (mean ± std. dev. of 7 runs, 100 loops each)  #PR
54.4 ms ± 244 µs per loop (mean ± std. dev. of 7 runs, 10 loops each)    #master

[simonjayhawkins]

does this also fix the case mentioned in #41596 (comment)? if so can you post the timings for that too for df_str.groupby('a')[cols[1:]].last() on master and with this PR

[debnathshoham]

Hi @simonjayhawkins - no this PR doesn't address the performance slowdown in the object dtype.
It only ensures that string takes the cython-path (similar to the object dtype).

Also note, that I have changed the size of the df in the top comment, wrt the example in the bug report.

[simonjayhawkins]

That's okay, but we can't close that issue. will need to open a new one specifically for that case, (regression between 1.2.5 and 1.3) or change the OP to read "xref" instead of "closes"

out of curiosity, did you identify which commit caused the slowdown between 1.1.5 and 1.2?

[debnathshoham]

Updated the top comment.
Honestly, I didn't try to find the commit. This fix was suggested in a comment in the bug report.

[simonjayhawkins]

So why do we need to special case an specific EA dtype? (Maybe this was done before). In general EA handling should be more generic.

[debnathshoham]

I think the current implementation of _ea_wrap_cython_operation works that way, no?

I can see a TODO for a more generic approach.

[simonjayhawkins]

yes, it appears that there is intent to make _ea_wrap_cython_operation more generic. I'm just wondering why it worked in 1.1.5

[jreback]

are there other functions (e.g. .sum) as well (or just have all string functions hit this path)? do we have sufficient asv's to cover this?

[debnathshoham]

yes, other functions do hit this path.
asv coverage is little spotty, i was not able to find anything with groupby and StringDtype

[jreback]

yes, other functions do hit this path.

ok which ones? what if you remove the self.how entirely

[jreback]

i don't really like the self.how here, this is way too specific.

[simonjayhawkins]

can you merge master to fix doc build #43688

[simonjayhawkins]

That's okay, but we can't close that issue. will need to open a new one specifically for that case, (regression between 1.2.5 and 1.3) or change the OP to read "xref" instead of "closes"

changed back to "closes" xref #41596 (comment)

[jreback]

can you add asvs that hit this case

[jreback]

groupby.String.time_str_func: setup: wrong number of arguments (for <bound method String.setup of <benchmarks.groupby.String object at 0x7f6c8c634a30>> in groupby.py:626): expected 2, has 1

in the asvs

[jreback]

also merge master as the deprecation warnings on asv's are silenced now

[jreback]

seems still some CI / checks are failing, also make sure you merge upstream/master as the patch for some of the deprecation warnings in asv's was not pulled in.

[jreback]

thanks @debnathshoham very nice!

[jreback]

@meeseeksdev backport 1.3.x

[jreback]

@debnathshoham if you wouldn't mind following the procedure above to backport