ENH: Only apply first group once in fast GroupBy.apply
State: closed

The issue of applying a function to the first row twice has been reported quite a few times (issues are usually referenced to #2936) and is also a documented shortcoming/implementation detail (see Notes in [docs](http://pandas.pydata.org/pandas-docs/stable/generated/pandas.DataFrame.apply.html))

The argumentation is usually that this needs to be done to determine whether or not we can take a fast path for the calculation. Even if we're using the fast path for the calculation, however, the first row is still evaluated twice. I believe with this refactoring it would be possible to only eval the first row once iff the fast path is taken.
For functions with side effects or very expensive calculations in general this may be a big deal as discussed in various other issues.

I'm wondering if this small change may already help out folks. Truth be told, I'm not 100% certain about what's actually happening in this loop regarding the slider and item cache. For the use case where I encountered this, it seems to do the trick, though.

closes #2936
closes #2656
closes #7739
closes #10519
closes #12155
closes #20084
closes #21417

- [x] tests added / passed
- [x] passes `git diff upstream/master -u -- "*.py" | flake8 --diff`
- [x] whatsnew entry


Labels:
name: Groupby
name: Performance
description: Memory or execution speed performance

Comments:
# [Codecov](https://codecov.io/gh/pandas-dev/pandas/pull/24748?src=pr&el=h1) Report
> Merging [#24748](https://codecov.io/gh/pandas-dev/pandas/pull/24748?src=pr&el=desc) into [master](https://codecov.io/gh/pandas-dev/pandas/commit/33f91d8f9f2e84f2b5f3ac3f0481b691c977c427?src=pr&el=desc) will **not change** coverage.
> The diff coverage is `n/a`.

[![Impacted file tree graph](https://codecov.io/gh/pandas-dev/pandas/pull/24748/graphs/tree.svg?width=650&token=eZ4WkYLtcO&height=150&src=pr)](https://codecov.io/gh/pandas-dev/pandas/pull/24748?src=pr&el=tree)

```diff
@@           Coverage Diff           @@
##           master   #24748   +/-   ##
=======================================
  Coverage   92.38%   92.38%           
=======================================
  Files         166      166           
  Lines       52358    52358           
=======================================
  Hits        48373    48373           
  Misses       3985     3985
```

| Flag | Coverage Δ | |
|---|---|---|
| #multiple | `90.81% <ø> (ø)` | :arrow_up: |
| #single | `43.07% <ø> (ø)` | :arrow_up: |


------

[Continue to review full report at Codecov](https://codecov.io/gh/pandas-dev/pandas/pull/24748?src=pr&el=continue).
> **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta)
> `Δ = absolute <relative> (impact)`, `ø = not affected`, `? = missing data`
> Powered by [Codecov](https://codecov.io/gh/pandas-dev/pandas/pull/24748?src=pr&el=footer). Last update [33f91d8...0102842](https://codecov.io/gh/pandas-dev/pandas/pull/24748?src=pr&el=lastupdated). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments).

# [Codecov](https://codecov.io/gh/pandas-dev/pandas/pull/24748?src=pr&el=h1) Report
> Merging [#24748](https://codecov.io/gh/pandas-dev/pandas/pull/24748?src=pr&el=desc) into [master](https://codecov.io/gh/pandas-dev/pandas/commit/51c6a05477ffb5835ee52fc0bbb26e2f64f13bf7?src=pr&el=desc) will **increase** coverage by `<.01%`.
> The diff coverage is `100%`.

[![Impacted file tree graph](https://codecov.io/gh/pandas-dev/pandas/pull/24748/graphs/tree.svg?width=650&token=eZ4WkYLtcO&height=150&src=pr)](https://codecov.io/gh/pandas-dev/pandas/pull/24748?src=pr&el=tree)

```diff
@@            Coverage Diff             @@
##           master   #24748      +/-   ##
==========================================
+ Coverage   91.48%   91.48%   +<.01%     
==========================================
  Files         175      175              
  Lines       52881    52885       +4     
==========================================
+ Hits        48376    48380       +4     
  Misses       4505     4505
```

| Flag | Coverage Δ | |
|---|---|---|
| #multiple | `90.04% <100%> (ø)` | :arrow_up: |
| #single | `41.82% <0%> (-0.01%)` | :arrow_down: |

| [Impacted Files](https://codecov.io/gh/pandas-dev/pandas/pull/24748?src=pr&el=tree) | Coverage Δ | |
|---|---|---|
| [pandas/core/groupby/ops.py](https://codecov.io/gh/pandas-dev/pandas/pull/24748/diff?src=pr&el=tree#diff-cGFuZGFzL2NvcmUvZ3JvdXBieS9vcHMucHk=) | `94.21% <100%> (+0.05%)` | :arrow_up: |

------

[Continue to review full report at Codecov](https://codecov.io/gh/pandas-dev/pandas/pull/24748?src=pr&el=continue).
> **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta)
> `Δ = absolute <relative> (impact)`, `ø = not affected`, `? = missing data`
> Powered by [Codecov](https://codecov.io/gh/pandas-dev/pandas/pull/24748?src=pr&el=footer). Last update [51c6a05...ecb7294](https://codecov.io/gh/pandas-dev/pandas/pull/24748?src=pr&el=lastupdated). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments).

I don't believe that changing the python code is actually sufficient since the Cython code I am modifying evaluates the first row twice, even if everything works as expected. My change here is essentially only using the calculation for the first row/test if the test is successful instead of recalculating it again.

I believe you are referring to https://github.com/pandas-dev/pandas/blob/453fa85a8b88ca22c7b878a3fcf97e068f11b6c4/pandas/core/groupby/ops.py#L174
where `fast_apply`(defined [here](https://github.com/pandas-dev/pandas/blob/453fa85a8b88ca22c7b878a3fcf97e068f11b6c4/pandas/core/groupby/ops.py#L848)) is called and if successful, the result (calculated in Cython) is immediately returned. If an `InvalidApply` exception is caught, a slow, python-only path is taken. I'm not trying to optimize the slow/python path but I am arguing that even the `fast_apply` path will evaluate the first row twice although this isn't necessary.

Apart from implicit tests (this code section should be called at many places) there is one explicit test to ensure that the fast path can be taken and the flags for `mutated` are returned as expected, see https://github.com/pandas-dev/pandas/blob/453fa85a8b88ca22c7b878a3fcf97e068f11b6c4/pandas/tests/groupby/test_apply.py#L81
If there are other edge cases you can think of I can add more tests.
@fjetter I would like to collect all tests from any of the references issues. After you add them see if this closes them; list the issues in the top of the PR.
Hello @fjetter! Thanks for updating this PR. We checked the lines you've touched for [PEP 8](https://www.python.org/dev/peps/pep-0008) issues, and found:







There are currently no PEP 8 issues detected in this Pull Request. Cheers! :beers: 

##### Comment last updated at 2019-03-26 03:47:26 UTC
@jreback Sorry for the delay. I went through the referenced issues and looked at the code examples the people where posting. Mostly the examples were pretty close to each other and I classified them into two categories (see below). Essentially most reported tickets were suggesting a very simple `DataFrame.groupby.apply` using a function which returns a scalar. This triggers the fast path which is what my change should fix. There was only a single issue where the example triggered the slow path computation by applying the identity.  (Type B: When using the identity there is a check in place which infers that the group may have been mutated which then triggers the fallback to the slow path)

I added a few more tests to make the current behavior more explicit, i.e. fast path applies row/group once but slow path still does it twice.
Although, I do have some ideas on how to improve the situation for the slow path; this would require larger changes and I do not see this happening in the scope of this PR. I believe improving the fast path alone is already offering value to the users.

Type A1 : Groupy DataFrame on col name; apply doesn't mutate (fast path)
Type A2 : Groupy DataFrame on group keys; apply doesn't mutate (fast path)
Type A3 : Groupy DataFrame on multiple names; apply doesn't mutate (fast path)

Type B: Apply DataFrame; apply identity

#2936 - Type A1 print / return new df
#2656 - Type A2 print / return len
#6753 - Type B - print / identity; slow path
#7739 - Type A1 - print / return 0
#10222 - Question
#10519 - Type A1 - print / return None
#12155 - Type A1 - print / return None
#16946 - Question
#20084 - Type A1 - return group.describe
#21417 - Type A3 - print / return None
also we have a warning in the groupby.rst about this, can remove it.
I added a whatsnew entry and added the issue references to the tests. I kept the warning in `groupby.rst` since the double func evaluation may still occur if we need to fall back to the slow path.
> I added a whatsnew entry and added the issue references to the tests. I kept the warning in groupby.rst since the double func evaluation may still occur if we need to fall back to the slow path.

that needs to be addressed here as well - i suspect this is actually the more common problem 
I changed the implementation such that also the slow path evaluates the function only once. I refactored `apply_frame_axis0` to signal the sucess/validity of the fast apply via a status code instead of an exception. If the status code is 1, we fall back to the slow path calculation but will use the already calculated result for the first group.
i am ok with slightly duplicative tests if they are testing subtle behavior - if they are exact duplicates then ok to combine
The `test_group_name_available_in_inference_pass` is now a subset of `test_group_apply_once_per_group` in `groupby/test_apply.py`. Although the implementation is very similar I'd keep them both since they test different behavior
pls merge master
can you merge master and fix the conflict
can you merge master
rebased & whatsnew updated.
@WillAyd if you have any more comments.
Thanks Jeff I’ll take a look tonight and merge if good

Sent from my iPhone

> On Mar 25, 2019, at 6:02 PM, Jeff Reback <notifications@github.com> wrote:
> 
> @WillAyd if you have any more comments.
> 
> —
> You are receiving this because you were mentioned.
> Reply to this email directly, view it on GitHub, or mute the thread.

CI failure looked unrelated. Merged master will merge on green
Thanks @fjetter !
Is this true only for groupby.apply? I tried test with just `apply` on a standard dataframe, and I found that the applied function is still being executed twice for the first row. This is quite problematic for me as my applied function is a side-effect.
@CMCDragonkai This issue is only fixed for the groupby.apply path. The ordinary `DataFrame.apply` uses different code paths where maybe a similar issue is buried.
Are you serious! So while groupby apply no longer has duplicate side effects, but normal apply still does!? Shouldn't it be easy to apply this fix to the situation for normal dataframe apply?
@CMCDragonkai I'd need to check what is causing the duplicate execution to say for sure. My gut tells me that it should be similarly possible. I can not guarantee it without actually checking it.
Are there any news regarding fixing this for the df.apply?
> Are you serious! So while groupby apply no longer has duplicate side effects, but normal apply still does!?

@CMCDragonkai this is no way to address a community contributor who has volunteered their time to submit a pull request, please refrain from using such a tone in the future - thank you :)