Fix Regression when using sum/cumsum on Groupby objects 
State: closed

- [x] closes #43292
- [x] tests added / passed
- [ ] Ensure all linting tests pass, see [here](https://pandas.pydata.org/pandas-docs/dev/development/contributing_codebase.html#pre-commit) for how to run them
- [ ] whatsnew entry

NOTE: This will reduce performance significantly. Hopefully someone points me to a better check for `nan` which can be done with released GIL.

Labels:
name: Groupby
name: Regression
description: Functionality that used to work in a prior pandas version
name: Stale

Comments:
Performance was reduced through introducing Kahan. I don't think we can reduce it further. Have to find a way around the gil
got around the gil. Now it's still the additional checks for `np.nan` and `-np.nan` in each iteration. But what are these failed checks about?
> got around the gil. Now it's still the additional checks for `np.nan` and `-np.nan` in each iteration. But what are these failed checks about?

Usually a build error when you see that. In this case: 
```
pandas/_libs/groupby.c:12588:18: error: ‘__pyx_v_t’ may be used uninitialized in this function [-Werror=maybe-uninitialized]
12588 |               if (__pyx_t_13) {
      |                  ^
pandas/_libs/groupby.c:12228:28: note: ‘__pyx_v_t’ was declared here
12228 |   __pyx_t_5numpy_float32_t __pyx_v_t;
      |                            ^~~~~~~~~
pandas/_libs/groupby.c: In function ‘__pyx_fuse_9__pyx_pw_6pandas_5_libs_7groupby_57group_cumsum’:
pandas/_libs/groupby.c:13372:18: error: ‘__pyx_v_t’ may be used uninitialized in this function [-Werror=maybe-uninitialized]
13372 |               if (__pyx_t_13) {
      |                  ^
pandas/_libs/groupby.c:13012:28: note: ‘__pyx_v_t’ was declared here
13012 |   __pyx_t_5numpy_float64_t __pyx_v_t;
      |                            ^~~~~~~~~
cc1: all warnings being treated as errors
```
What do the benchmarks look like here? Since we already have the regression #39622 from using kahan (and this adds decent complexity), would like to propose returning to previous simple summation for 1.3.5, which would fix both that regression and this one. Maybe we could expose kahan summation as an optional keyword instead (which a user can just know not to use if their data has infs?)
> What do the benchmarks look like here? Since we already have the regression #39622 from using kahan (and this adds decent complexity), would like to propose returning to previous simple summation for 1.3.5, which would fix both that regression and this one. Maybe we could expose kahan summation as an optional keyword instead (which a user can just know not to use if their data has infs?)

Here are the performance measures:
``` python
import numpy as np
import pandas as pd
import timeit

arr = np.ones(1000000)
s = pd.Series(np.arange(len(arr)))
s = s / 10

%timeit -r7 -n1000 s.groupby(arr).sum()
# on this branch
# 15.4 ms ± 172 µs per loop (mean ± std. dev. of 7 runs, 1000 loops each)
# on 1.3.4
# 15.4 ms ± 119 µs per loop (mean ± std. dev. of 7 runs, 1000 loops each)

idx = pd.date_range(start="1/1/2000", end="1/1/2001", freq="T")
s = pd.Series(np.random.randn(len(idx)), index=idx)
%timeit -r7 -n1000 s.resample("1D").mean()
# on this branch
# 4.67 ms ± 44.4 µs per loop (mean ± std. dev. of 7 runs, 1000 loops each)
# on 1.3.4
# 4.61 ms ± 35.9 µs per loop (mean ± std. dev. of 7 runs, 1000 loops each)
```
So there is a performance regression of 2-3%
Maybe it would be the best option just to go back to normal summation and expose kahan's summation as a keyword. If the user decides to take the slower but numerically more stable summation, I guess he/she can affort the extra time to get the correct results for the special case with `np.inf`.
Could you please run the asvs?
After running the asv, this is a remarkable performance hit:
```python
from asv_bench.benchmarks.groupby import *
self = GroupByCythonAgg()
self.setup("float64", "sum")
%timeit -r 30 -n 100 self.time_frame_agg("float64", "sum")
# this PR
# 29.7 ms ± 257 µs per loop (mean ± std. dev. of 30 runs, 100 loops each)
# master
# 20.1 ms ± 279 µs per loop (mean ± std. dev. of 30 runs, 100 loops each)
```
> What do the benchmarks look like here? Since we already have the regression #39622 from using kahan (and this adds decent complexity), would like to propose returning to previous simple summation for 1.3.5, which would fix both that regression and this one. Maybe we could expose kahan summation as an optional keyword instead (which a user can just know not to use if their data has infs?)

needs a release note. adding the 1.3.5 milestone to match the linked issue.

thoughts on backporting these changes?


move this off 1.3.x it's too late to change anything
Thanks @jbrockmendel for pointing me to the isinf function from numpy. This also influences the performance:

```python
In [1]: from asv_bench.benchmarks.groupby import *

In [2]: self = GroupByCythonAgg()

In [3]: self.setup("float64", "sum")

In [4]: %timeit -r 30 -n 100 self.time_frame_agg("float64", "sum")
# This PR
# 25.3 ms ± 377 µs per loop (mean ± std. dev. of 30 runs, 100 loops each)
# master
# 19.8 ms ± 355 µs per loop (mean ± std. dev. of 30 runs, 100 loops each)
```
This pull request is stale because it has been open for thirty days with no activity. Please update or respond to this comment if you're still interested in working on this.
Thanks for the PR, but it appears to have gone stale. If interested in continuing please merge the main branch  and we can reopen.