CLN: Unify signatures in _libs.groupby
State: closed

- [ ] closes #xxxx
- [ ] tests added / passed
- [x] passes `black pandas`
- [x] passes `git diff upstream/master -u -- "*.py" | flake8 --diff`
- [ ] whatsnew entry

xref https://github.com/pandas-dev/pandas/pull/33630#discussion_r428737327

The main goal is to allow _get_cythonized_result to be used with the cython implementation of var. In order to do this, it was best to modify the signatures in _libs.groupby of 

 - group_any_all;
 - _group_var; and
 - group_quantile

so that the order of the arguments matches other cython functions (e.g. those used in ops.BaseGrouper.aggregate or transform).

Note this PR has the fix for #33955 that is also in #33988 - that PR should be merged first.

Tests using %timeit on dataframes of various shapes (code for generating is at the bottom):

![image](https://user-images.githubusercontent.com/45562402/83825639-e3cf8a00-a6a7-11ea-84c7-c224870cae8b.png)

I looked into the last row. 26.7% of the time is spent in just iterating over the columns, 12.8% in the cython call, and 35.7% in the call to ``_wrap_aggregated_output``.

While working on this, it seemed best to implement ``ddof != 1`` in cython for var. The following code takes 26.5ms on master and 3.88ms in this PR.

````
ncol, order = 1, 6
df = pd.DataFrame(np.random.uniform(low=0.0, high=10.0, size=(10**order, ncol)))
df['key'] = np.random.randint(0, 50, (10**order, 1))
g = df.groupby('key')
t = %timeit -o g.std(ddof=4)
````

Labels:
name: Refactor
description: Internal refactoring of code
name: Groupby

Comments:
@WillAyd I got everything working until I tried to use it for var, which was the original motivation. For var, tests with some columns being nullable integers seemed not possible to make work. So instead of changing the cython implementation of quantile et al to use a 2d input/output, I instead made _get_cythonized_result more flexible to be able to work with cython implementations that are meant for 2d data. More details in the OP.
@WillAyd Here are the significant asv changes using "-b ^groupby"

````
       before           after         ratio
     [c71bfc36]       [98f1c0d2]
     <at_duplicate_labels>       <_get_cython_result>
+      7.89±0.4ms       9.38±0.5ms     1.19  groupby.Nth.time_series_nth('float32')
+         413±2ms         472±20ms     1.14  groupby.Apply.time_copy_overhead_single_col
+       101±0.7μs          112±4μs     1.12  groupby.GroupByMethods.time_dtype_as_field('datetime', 'count', 'direct')
+     6.70±0.03ms       7.47±0.6ms     1.12  groupby.Transform.time_transform_multi_key1
+      98.9±0.5μs          109±4μs     1.10  groupby.GroupByMethods.time_dtype_as_group('object', 'count', 'direct')
-        268±40μs          233±2μs     0.87  groupby.GroupByMethods.time_dtype_as_group('object', 'ffill', 'direct')
-         622±3μs         523±10μs     0.84  groupby.GroupByMethods.time_dtype_as_field('float', 'sem', 'transformation')
-         627±4μs         522±10μs     0.83  groupby.GroupByMethods.time_dtype_as_field('float', 'sem', 'direct')
-         685±4μs          540±8μs     0.79  groupby.GroupByMethods.time_dtype_as_group('float', 'sem', 'transformation')
-         684±4μs          533±8μs     0.78  groupby.GroupByMethods.time_dtype_as_group('float', 'sem', 'direct')
-        678±20μs          516±1μs     0.76  groupby.GroupByMethods.time_dtype_as_field('int', 'sem', 'direct')
-        696±40μs          526±7μs     0.76  groupby.GroupByMethods.time_dtype_as_field('int', 'sem', 'transformation')
-         708±4μs          528±2μs     0.75  groupby.GroupByMethods.time_dtype_as_group('int', 'sem', 'transformation')
-         710±8μs        525±0.5μs     0.74  groupby.GroupByMethods.time_dtype_as_group('int', 'sem', 'direct')
-        348±60μs          256±3μs     0.74  groupby.GroupByMethods.time_dtype_as_group('object', 'last', 'direct')
-       306±0.8μs          194±8μs     0.63  groupby.GroupByMethods.time_dtype_as_field('float', 'std', 'transformation')
-         305±2μs          192±3μs     0.63  groupby.GroupByMethods.time_dtype_as_field('float', 'std', 'direct')
-         366±7μs          200±4μs     0.55  groupby.GroupByMethods.time_dtype_as_group('float', 'std', 'direct')
-         362±4μs          197±1μs     0.54  groupby.GroupByMethods.time_dtype_as_group('float', 'std', 'transformation')
-         362±5μs          191±1μs     0.53  groupby.GroupByMethods.time_dtype_as_field('int', 'std', 'direct')
-         367±8μs          191±1μs     0.52  groupby.GroupByMethods.time_dtype_as_field('int', 'std', 'transformation')
-         387±6μs          197±2μs     0.51  groupby.GroupByMethods.time_dtype_as_group('int', 'std', 'direct')
-         390±8μs        197±0.7μs     0.51  groupby.GroupByMethods.time_dtype_as_group('int', 'std', 'transformation')
````

Edit: New to using asv - I ran it using the command

    asv continuous -f 1.1 upstream/master HEAD -b ^groupby

but it pulled in the branch name "at_duplicate_labels"? I don't think this is of any consequence, this branch is identical to master.
@jreback Friendly ping. In https://github.com/pandas-dev/pandas/pull/34372#discussion_r437052681 you asked if the extraction of name might be used anywhere else. Not that I can tell from searching, but it's possible I've missed something. I think that means hold off making it a property? Also note that the code to extract it has now been simplified to:

    name = self._obj_with_exclusions._get_axis(1 - self.axis).name
> Note this PR has the fix for #33955 that is also in #33988 - that PR should be merged first.

is this comment still relevant?


@jreback - changes made, still waiting on checks. I assumed you wanted to keep the prefix "needs" to that the flag is now "needs_at_least2d".

> are there any perf implications here?

asv is here: https://github.com/pandas-dev/pandas/pull/34372#issuecomment-640217728
The OP also has timings using %timeit for frames of various shapes: https://github.com/pandas-dev/pandas/pull/34372#issue-422910411

I think the last line of the timings in the OP is the only one of concern, where on a short and wide frame var is taking 17x longer. I also profiled this using line_profiler. From the OP:

``
I looked into the last row. 26.7% of the time is spent in just iterating over the columns, 12.8% in the cython call, and 35.7% in the call to _wrap_aggregated_output.
``

> > Note this PR has the fix for #33955 that is also in #33988 - that PR should be merged first.
> 
> is this comment still relevant?

Unclear - that PR has the fix that is also in here, but it's never hit by any tests there. This PR on the other hand has hits the change via tests involving std. Just don't want to step on any toes, I only saw that PR as I was submitting this one. 
ok @rhshadrach happy to merge this, pls open an issue for the perf issue  (on short / wide) can address in a followup.
@rhshadrach can you merge master? Hopefully the CI issues have all been resolved.
@TomAugspurger thanks!
@jreback all green now.
@jreback Thanks for the comments, changes made and checks pass.
thanks @rhshadrach nice cleanup