BUG: Fix groupby.apply
State: closed

- [x] closes #28652
- [x] tests added / passed
- [x] passes `black pandas`
- [x] passes `git diff upstream/master -u -- "*.py" | flake8 --diff`
- [x] whatsnew entry

Makes sure that the output of `groupby.apply` is built up by value instead of by reference in `reduction.pyx` to avoid the behavior from #28652.

Labels:
name: Bug
name: Groupby
name: Indexing
description: Related to indexing on series/frames, not to indexes themselves

Comments:
Somewhat unrelated, but does `isort` skip over `pyx` files during checks?  I've noticed it doesn't like the import order / formatting of `reduction.pyx`.
> Somewhat unrelated, but does `isort` skip over `pyx` files during checks? I've noticed it doesn't like the import order / formatting of `reduction.pyx`.

Correct the --recursive option only actually checks `.py` and `.pyi` files. As per isort code [here](https://github.com/alimcmaster1/isort/blob/develop/isort/main.py#L78).

Even though one can explicitly sort a given pyx file eg. running `isort pandas/_libs/reduction.pyx` works. Maybe @timothycrosley knows if this is expected behaviour?

cc. @jbrockmendel to comment on if its worth us bothering to isort `.pyx` files?
@alimcmaster1, yes this is intended, for now. There has been an open ticket for some time to add full Cython awareness (cimports, cdefs, etc...) but for now it remains incomplete. Because of this, we currently intentionally don't automatically include pyx files. Once this ticket is resolved, isort will include pyx files by default.

Hope this is helpful!

~Timothy
> to comment on if its worth us bothering to isort .pyx files?

I've tried to make these internally consistent, but isort wont work on these since they are not valid python.

<b>update</b> looks like I was wrong about isort not working on them. still for now "get it close enough manually" makes sense to me
lgtm. can you rebase and small comment. ping on green.
Another approach we could take would be to update some combination of libreduction._check_result_array and libreduction._extract_result to check for Index objects and redirect us back to the pure-python implementation, which already does this corrrectly.
@dsaxton - feels like this is almost there? Seems like the test failures mentioned above are now resolved.
can you merge master and will look again
thanks @dsaxton 

not generally happy with the way we do reductions in cython, but a larger discussion / refactor effort.
@dsaxton this caused a performance regression for some of our benchmarks, eg https://pandas.pydata.org/speed/pandas/#groupby.Apply.time_copy_overhead_single_col?commits=6efc2379-b9de33e3. Now, it might be this is unavoidable since the extra copy is to avoid a bug, but can you take a look at that benchmark?
> @dsaxton this caused a performance regression for some of our benchmarks, eg https://pandas.pydata.org/speed/pandas/#groupby.Apply.time_copy_overhead_single_col?commits=6efc2379-b9de33e3. Now, it might be this is unavoidable since the extra copy is to avoid a bug, but can you take a look at that benchmark?

@jorisvandenbossche Interesting, it looks like the copy was previously avoided because each group was getting copied by the function in the benchmark, so the identity check on the indexes wasn't triggering (`if piece.index is chunk.index`).

Not really sure what the best approach would be here; would you recommend adding  an explicit check like `piece.index is not chunk.index` somewhere so that we know copying isn't necessary, or is that getting a bit too messy?