ENH: Enabled skipna argument on groupby reduction ops #15675
State: closed

- [x] closes #15675
- [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/v3.0.0.rst` file if fixing a bug or adding a new feature.

Added a skipna argurment to the groupby reduction ops for consistency with the Series and Dataframe variants:
- sum, 
- prod, 
- min, 
- max, 
- mean, 
- median, 
- var, 
- std,
- sem

Added new relevant tests, updated api tests and whatsnew


Labels:
name: Groupby
name: Stale

Comments:
Hi, we refactored the tests as requested and were working on adding the EAs. We found and fixed a few sneaky edge case bugs with these, but we ran into a problem with ```dtype=pd.ArrowDtype(pa.int64())```.

The current implementation with EAs creates a typed numpy array and no NA value can be directly used in place for integers, we don't really see a path forward without considerable changes for this particular dtype.

We could add arrow floats easily, but we felt like only having a few arrow dtypes supported doesn't make much sense. 

How should we proceed?

@rhshadrach 
> Since the current implementation with arrows requires creating a typed numpy array and no NA value can be directly used in place for integers, we don't really see a path forward without considerable changes for this particular dtype.

The groupby methods implemented in Cython use `mask` and `result_mask` arguments for this purpose. This indicates where NA values are in the input and output respectively, so that they don't need to be in the NumPy array. If this isn't clear I can flesh it out some more, just ask.

I can also open up a PR into your branch if you want some assistance with the EAs here.
Hello, after some rethinking about how we were handling EAs, we believe we've got it done correctly now since we were overthinking it before. However, we noticed the relevant array types tend to use ```pd.NA``` whilst the Series variants we are using to compare for our expected values are inferring ```np.nan```. 
We would like to know what to do in this situation, that is, what should be the expected value in EAs case, np.nan or pd.NA.

Our way of testing it was as such:
````

df = DataFrame({"key": [1, 1, 1, 2, 2, 2], "values": Series(pd.array([-1.0, 1.2, -1.1, 1.5, np.nan, 1.0], dtype="Float64"))})
gb = df.groupby("key")
result_cython = getattr(gb, reduction_method)(skipna=False)
expected = gb.apply(
        lambda x: getattr(x, reduction_method)(skipna=False), include_groups=False
)
tm.assert_frame_equal(result_cython, expected, check_exact=False, check_dtype=False)
`````
And the ```AssertionError``` we get is:
```
[-0.9000000000000001, <NA>]
E   Length: 2, dtype: Float64
E   [right]: [-0.9000000000000001, nan]
E   At positional index 1, first diff: <NA> != nan
```

In relation to the tests in ```tests/extension/base/reduce.py```, we would appreciate if some help with understanding how the tests work, how they are called and where the arguments come from. We understood they are the foundation for other tests used in the ```tests/extension``` directory but some fields left us confused.
> [tiago-firmino](https://github.com/tiago-firmino) [force-pushed](https://github.com/pandas-dev/pandas/compare/558bc25b1d3ed9d96537f78413948526ac8883fd..2856c6dd1a6451423bdcaea6ed2e92a325f84168) the add_skipna_on_groupby_ops_pr branch from[ 558bc25 ](https://github.com/pandas-dev/pandas/commit/558bc25b1d3ed9d96537f78413948526ac8883fd)to 2856c6d

@tiago-firmino - each time you force push, reviewers must review the entire PR as history could be changing. On small PRs, this isn't much of a problem, but this is not a small PR. Can I ask that you no longer force push here?
> However, we noticed the relevant array types tend to use `pd.NA` whilst the Series variants we are using to compare for our expected values are inferring `np.nan`.

Indeed, `apply` here looks to be producing the wrong result.

> In relation to the tests in `tests/extension/base/reduce.py`, we would appreciate if some help with understanding how the tests work, how they are called and where the arguments come from.

For this PR, the relevant tests are in `tests/extension/base/reduce.py` in the class `BaseReduceTests`. In `tests/extension/base/__init__.py`, the class `ExtensionTests` is then a subclass of this, so inherits its methods. pytest runs the files e.g. `tests/extension/test_arrow.py` where there is the class `TestArrowArray(base.ExtensionTests)`. This class has all the methods of `BaseReduceTests`, so the tests are run. Within `test_arrow.py` are fixtures such as `data` which setup data specifically for the pyarrow tests. Likewise, in `test_masked.py`, the fixture `data` sets up data specifically for the NumPy-nullable tests.


> > [tiago-firmino](https://github.com/tiago-firmino) [force-pushed](https://github.com/pandas-dev/pandas/compare/558bc25b1d3ed9d96537f78413948526ac8883fd..2856c6dd1a6451423bdcaea6ed2e92a325f84168) the add_skipna_on_groupby_ops_pr branch from[ 558bc25 ](https://github.com/pandas-dev/pandas/commit/558bc25b1d3ed9d96537f78413948526ac8883fd)to 2856c6d
> 
> @tiago-firmino - each time you force push, reviewers must review the entire PR as history could be changing. On small PRs, this isn't much of a problem, but this is not a small PR. Can I ask that you no longer force push here?

I'm really sorry, I was not aware of that and will no longer do it.
Hello,
We've had a bit less free time lately, hence the delay, but we've run into some problems that we're not sure how to address. 
The main issue is with the prod function using int8, int16, and int32. Locally, we can't replicate the errors that show up in the pipeline, but we were able to identify that the wrong value is in the expected output, not in our result. However, after spending a long time looking at this issue, neither of us has been able to find a solution. 
Do you have any suggestions on how we could tackle this?
@andremcorreia - I should be able to take a look in the next few days.
@andremcorreia 

> but we were able to identify that the wrong value is in the expected output, not in our result.

It looks to me like the `result` is overflowing in 64-bit precision, the expected in 32-bit precision. On Windows, this will be fixed by NumPy 2.0. In any case, this isn't an issue with this PR. Can you add the following to the corresponding test:

```python
if op_name == "prod" and skipna and data.dtype.itemsize < 8 and np.intp().itemsize < 8:
    pytest.xfail(reason=f"{op_name} with itemsize {data.dtype.itemsize} overflows")
```
This pull request is stale because it has been open for thirty days with no activity. Please [update](https://pandas.pydata.org/pandas-docs/stable/development/contributing.html#updating-your-pull-request) and respond to this comment if you're still interested in working on this.
Thanks for the pull request, but it appears to have gone stale. If interested in continuing, please merge in the main branch, address any review comments and/or failing tests, and we can reopen.