BUG: Fix groupby.apply

[dsaxton]

• closes groupby indexing is giving the wrong index #28652
• tests added / passed
• passes black pandas
• passes git diff upstream/master -u -- "*.py" | flake8 --diff
• 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.

[dsaxton]

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.

[alimcmaster1]

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.

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?

[timothycrosley]

@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

[jbrockmendel]

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.

update looks like I was wrong about isort not working on them. still for now "get it close enough manually" makes sense to me

[WillAyd]

lgtm @jreback

[jreback]

lgtm. can you rebase and small comment. ping on green.

[jbrockmendel]

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.

[alimcmaster1]

@dsaxton - feels like this is almost there? Seems like the test failures mentioned above are now resolved.

[jreback]

can you merge master and will look again

[jreback]

thanks @dsaxton

not generally happy with the way we do reductions in cython, but a larger discussion / refactor effort.

[jorisvandenbossche]

@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]

@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?