PERF: Implement groupby idxmax/idxmin in Cython
State: closed

- [x] closes #10694 (Replace xxxx with the GitHub issue number)
- [x] [Tests added and passed](https://pandas.pydata.org/pandas-docs/dev/development/contributing_codebase.html#writing-tests) if fixing a bug or adding a new feature
- [x] All [code checks passed](https://pandas.pydata.org/pandas-docs/dev/development/contributing_codebase.html#pre-commit).
- [x] Added [type annotations](https://pandas.pydata.org/pandas-docs/dev/development/contributing_codebase.html#type-hints) to new arguments/methods/functions.
- [x] Added an entry in the latest `doc/source/whatsnew/vX.X.X.rst` file if fixing a bug or adding a new feature.

Adds Cython code for idxmin/idxmax in groupby.

<details>
<summary>Code</summary>

```
for size in [100, 1_000, 10_000, 100_000, 1_000_000, 10_000_000]:
    df = pd.DataFrame(
        {
            'a': np.random.randint(0, 100, size),
            'b': np.random.random(size),
        }
    )
    gb = df.groupby('a')
    print(size)
    %timeit gb.idxmin()
    print()
```
</details>

```
size: 100
7.91 ms ± 29.8 µs per loop (mean ± std. dev. of 7 runs, 100 loops each)   <-- main
53.2 µs ± 121 ns per loop (mean ± std. dev. of 7 runs, 10,000 loops each) <-- PR

size: 1000
12.6 ms ± 28.3 µs per loop (mean ± std. dev. of 7 runs, 100 loops each)   <-- main
52.4 µs ± 513 ns per loop (mean ± std. dev. of 7 runs, 10,000 loops each) <-- PR

size: 10000
12.6 ms ± 13.6 µs per loop (mean ± std. dev. of 7 runs, 100 loops each)   <-- main
67.9 µs ± 115 ns per loop (mean ± std. dev. of 7 runs, 10,000 loops each) <-- PR

size: 100000
13 ms ± 9.81 µs per loop (mean ± std. dev. of 7 runs, 100 loops each)   <-- main
207 µs ± 805 ns per loop (mean ± std. dev. of 7 runs, 1,000 loops each) <-- PR

size: 1000000
18.3 ms ± 264 µs per loop (mean ± std. dev. of 7 runs, 100 loops each)   <-- main
1.56 ms ± 2.78 µs per loop (mean ± std. dev. of 7 runs, 1,000 loops each) <-- PR

size: 10000000
188 ms ± 178 µs per loop (mean ± std. dev. of 7 runs, 1 loop each)   <-- main
15.8 ms ± 31.7 µs per loop (mean ± std. dev. of 7 runs, 100 loops each) <-- PR
```

Added ASVs (the min line is just noise I think)

```
       before           after         ratio
     [3c01ce2a]       [73927a73]
     <gb_mi_cat_result_index>       <gb_idxmax_unobserved_cat>
-      15.5±0.2ms       13.8±0.2ms     0.89  groupby.GroupByCythonAgg.time_frame_agg('float64', 'min')
-      78.4±0.6ms       13.9±0.3ms     0.18  groupby.GroupByCythonAgg.time_frame_agg('float64', 'idxmin')
-      79.5±0.7ms       13.1±0.4ms     0.16  groupby.GroupByCythonAgg.time_frame_agg('float64', 'idxmax')
```

Labels:
name: Groupby
name: Performance
description: Memory or execution speed performance
name: Reduction Operations
description: sum, mean, min, max, etc.

Comments:
Looks really similar to #52339 but you got it working!
Ahhh, shoot! I forgot you had been working on this. Couple of minor differences and things I'll steal from yours if we're going forward with this (e.g. argmin/argmax instead of idxmin/idxmax; initializing `out` to -1 in Cython rather than where I have it here).

But I think the main difference is the use of post_processing here vs calling `_cython_agg_general` in yours. What do you think about the additional argument?
> But I think the main difference is the use of post_processing here vs calling _cython_agg_general in yours. What do you think about the additional argument?

bc it is only passed from _idxmin_idxmax, could we avoid having the argument at all and just do the post-processing there?
> > But I think the main difference is the use of post_processing here vs calling _cython_agg_general in yours. What do you think about the additional argument?
> 
> bc it is only passed from _idxmin_idxmax, could we avoid having the argument at all and just do the post-processing there?

That was my first attempt. The trouble is we do a bunch of post-processing already in `_cython_agg_general` (handling observed, as_index in particular) that gets in the way, and then we need to repeat this in idxmin/idxmax. Using the post_processing seems cleaner, at least with the current state.
> Using the post_processing seems cleaner, at least with the current state.

Seems reasonable, thanks for taking a look.
im not clear on how this fixes #10694.  isnt raising bc of an empty sequence correct there?
> im not clear on how this fixes #10694. isnt raising bc of an empty sequence correct there?

I keep going back and forth over whether no observations should be NA or raise. While working on this it seemed like making this raise effectively makes `observed=False` useless for idxmin/idxmax, and that many (all?) other ops fill in with NA if no other sentinels make sense. But thinking about this again, it should agree with e.g. `df.idxmin()` where `df` has no rows - I'm going to change this so it raises instead.


@jbrockmendel - I think this is in a better state now. One thing that needs to be done is decide how to handle `skipna`. This argument is only on a handful of groupby ops (skew, idxmin, idxmax, any, all, cum*). I'd suggest deprecate the argument (and treat as always True, the current default). If we go that route, I can put up a precursor PR for it.
> I'd suggest deprecate the argument (and treat as always True, the current default)

i'm open to being convinced, but id expect skipna to be present to mirror the NDFrame methods
@jbrockmendel - I believe this is ready for another look. I've changed the implementation to use `-2` in the codes. See the [OP](https://github.com/pandas-dev/pandas/pull/54234#issue-1817186156) with a description.

> i'm open to being convinced, but id expect skipna to be present to mirror the NDFrame methods

skipna is now implemented
@WillAyd @jbrockmendel - friendly ping.
Thanks @WillAyd - changes made.
This is a very meaty PR.  Are the test changes for changed/fixed behavior or just fleshing out the test suite?  If the latter, could split that into a precursor PR?
> Are the test changes for changed/fixed behavior or just fleshing out the test suite? If the latter, could split that into a precursor PR?

The only one that can be moved to a precursor is the added test `test_idxmin_idxmax_skipna_false`. All the rest are fixing the behavior with unobserved categoricals as mentioned in the OP.

I think I see way a to fix the behavior in a precursor and then this can be test-neutral. I'll give that a go.


@jbrockmendel @WillAyd: The change I implemented in #55268 was able to determine when there are unobserved categories without using a -2 sentinel nor a mask. Even better, we can determine this prior to the computation. It also enables us to call a dedicated method on GroupBy to wrap the result (`_wrap_idxmin_idxmax`) rather than introduce a `post_processing` argument. I don't have a preference here one way or the other.
Wonder if the crash is related. @lithomas1 would gcc / clang build warnings show up in CI? Or does meson hide that by default?
@WillAyd - I can reproduce locally.
Ah ok - do you get any build warnings when using --config-settings editable-verbose=true ?
> Ah ok - do you get any build warnings when using --config-settings editable-verbose=true ?

I believe I fixed this issue - is there still value in answering this (even just for curiosity)? I can revert my fix and try it out if so.
Well if you see any warnings I would treat them as errors (in the past we used to have -Werror in the CI), but if you didn't see any and your patch works no need to go back and investigate further
> Well if you see any warnings I would treat them as errors (in the past we used to have -Werror in the CI), but if you didn't see any and your patch works no need to go back and investigate further

I think we got rid of this because there were a lot of spurious ones. 

You should still be able to see build warnings, though.
(I'm not too sure about this, though).
@jbrockmendel - friendly ping
will prioritize this Monday AM
Couple of questions about reachability, otherwise LGTM
LGTM
Nice! Thanks @rhshadrach 
thanks for seeing this through @rhshadrach!