apply_frame_axis0 applies function to first group twice
State: closed

- [x] closes #21609
- [x] tests added / passed
- [x] passes `git diff upstream/master -u -- "*.py" | flake8 --diff`
- [x] whatsnew entry

Labels:
name: Groupby
name: Visualization
description: plotting

Comments:
Sorry for the git history mess
Don’t worry about the git history.  We’ll definitely need a test though.
It passes pytest locally, but has failed travis/appveyor. As of the test it's not clear to me how to do it. The extra plot does not produce a handle (3 plots lead to 2 axes objects), so there's no straightforward way to count it. Basically the function  gets called, but no trace of it is in the output of `apply_frame_axis0`/`fast_apply`. Does using some sort of static flags and incrementing them in cython seem like a good idea? Or possibly a private flag in `fast_apply`? Any suggestions? 
> It passes pytest locally, but has failed travis/appveyor

Did you run the test suite in both py2 and py3?  Linux or OSX or Windows?

Sometimes there are Travis failures unrelated to the PR, which are often solved by just re-pushing to make Travis run again.

Plots in general are tough to test.  If you can get a test working using pytest-mpl that could be _really_ helpful.
It was in py3 OSX. I'm repushing to see if that works. 

I'll try to come up with a test for this in the next couple of days. 
I wrote a test that checks the number of runs for an arbitrary function (demean). It may be possible to write something similar for plots with class inheritance from pd.core.frame.DataFrame or instead use the suggested pytest-mpl. But given that this test already captures the issue in a more general sense, do we need an explicit test for scatter plots?

I'm still not sure why so many tests keep failing, but there seem to exist lines such as the following which failed by pytest:
https://github.com/pandas-dev/pandas/blob/de664e82086dc147ac63a09848f0b2f13befde5f/pandas/tests/groupby/test_groupby.py#L1398

I suspect there may be more cases like that. It seems to me that these need to be analyzed one by one and removed if the PR is expected go through. Should I go ahead and remove the ones that seem to be conflicting? I'm worried this may have too many unwanted consequences.
Hello @javadnoorb! Thanks for updating the PR.









Cheers ! There are no PEP8 issues in this Pull Request. :beers: 

##### Comment last updated on August 06, 2018 at 20:54 Hours UTC
@jreback, sorry, that parameter was just for debugging. I'll remove in the next commit. Some tests related in tests/groupby fail. I'll make a commit as soon as I find out the reason. 

Regarding your comment, I suspect you're referring to this line:
https://github.com/pandas-dev/pandas/blob/1f6ddc4592728c1a09fc0a81ca2a00c685b60a8e/pandas/core/groupby/ops.py#L181

But the two calls to f(group #0) don't happen in fast_apply. They happen inside apply_frame_axis0:
First here:
https://github.com/pandas-dev/pandas/blob/1f6ddc4592728c1a09fc0a81ca2a00c685b60a8e/pandas/_libs/reduction.pyx#L515
and then here (when i=0):
https://github.com/pandas-dev/pandas/blob/1f6ddc4592728c1a09fc0a81ca2a00c685b60a8e/pandas/_libs/reduction.pyx#L532


I think PR name was confusing. I renamed it.
# [Codecov](https://codecov.io/gh/pandas-dev/pandas/pull/21943?src=pr&el=h1) Report
> Merging [#21943](https://codecov.io/gh/pandas-dev/pandas/pull/21943?src=pr&el=desc) into [master](https://codecov.io/gh/pandas-dev/pandas/commit/7725fa028aad7a8817c10b7fe67a34cf7fe8ee5c?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/21943/graphs/tree.svg?token=eZ4WkYLtcO&width=650&height=150&src=pr)](https://codecov.io/gh/pandas-dev/pandas/pull/21943?src=pr&el=tree)

```diff
@@            Coverage Diff             @@
##           master   #21943      +/-   ##
==========================================
+ Coverage   92.06%   92.06%   +<.01%     
==========================================
  Files         169      169              
  Lines       50694    50696       +2     
==========================================
+ Hits        46671    46675       +4     
+ Misses       4023     4021       -2
```

| Flag | Coverage Δ | |
|---|---|---|
| #multiple | `90.47% <100%> (ø)` | :arrow_up: |
| #single | `42.32% <50%> (ø)` | :arrow_up: |

| [Impacted Files](https://codecov.io/gh/pandas-dev/pandas/pull/21943?src=pr&el=tree) | Coverage Δ | |
|---|---|---|
| [pandas/core/groupby/ops.py](https://codecov.io/gh/pandas-dev/pandas/pull/21943/diff?src=pr&el=tree#diff-cGFuZGFzL2NvcmUvZ3JvdXBieS9vcHMucHk=) | `96.57% <ø> (ø)` | :arrow_up: |
| [pandas/core/groupby/groupby.py](https://codecov.io/gh/pandas-dev/pandas/pull/21943/diff?src=pr&el=tree#diff-cGFuZGFzL2NvcmUvZ3JvdXBieS9ncm91cGJ5LnB5) | `96.15% <100%> (ø)` | :arrow_up: |
| [pandas/core/groupby/base.py](https://codecov.io/gh/pandas-dev/pandas/pull/21943/diff?src=pr&el=tree#diff-cGFuZGFzL2NvcmUvZ3JvdXBieS9iYXNlLnB5) | `91.3% <100%> (+0.19%)` | :arrow_up: |
| [pandas/core/strings.py](https://codecov.io/gh/pandas-dev/pandas/pull/21943/diff?src=pr&el=tree#diff-cGFuZGFzL2NvcmUvc3RyaW5ncy5weQ==) | `98.63% <0%> (ø)` | :arrow_up: |
| [pandas/io/common.py](https://codecov.io/gh/pandas-dev/pandas/pull/21943/diff?src=pr&el=tree#diff-cGFuZGFzL2lvL2NvbW1vbi5weQ==) | `70.65% <0%> (ø)` | :arrow_up: |
| [pandas/tseries/offsets.py](https://codecov.io/gh/pandas-dev/pandas/pull/21943/diff?src=pr&el=tree#diff-cGFuZGFzL3RzZXJpZXMvb2Zmc2V0cy5weQ==) | `97.15% <0%> (ø)` | :arrow_up: |
| [pandas/plotting/\_core.py](https://codecov.io/gh/pandas-dev/pandas/pull/21943/diff?src=pr&el=tree#diff-cGFuZGFzL3Bsb3R0aW5nL19jb3JlLnB5) | `83.63% <0%> (+0.14%)` | :arrow_up: |

------

[Continue to review full report at Codecov](https://codecov.io/gh/pandas-dev/pandas/pull/21943?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/21943?src=pr&el=footer). Last update [7725fa0...1058066](https://codecov.io/gh/pandas-dev/pandas/pull/21943?src=pr&el=lastupdated). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments).

Still figuring out pytest-mpl
The pytest-mpl test works locally (on a separate script file). How/where should I add it? Where does the baseline file go?
I may have jumped the gun by suggesting pytest-mpl.  If you have another way to test the relevant behavior, go for it.
Ok, I'm gonna comment out the pytest-mpl for now to make sure the other tests pass, and wait for feedback on it. Catching the extra graph is not easy without manipulating plot functions (adding counters) so I actually don't think the test adds much beyond the one I've already written in tests/groupby.
For the record, after the changes, running the following in Jupyter lab:

```python
%matplotlib inline
import pandas as pd
df = pd.DataFrame([[1, 2], [3, 4], [5, 6], [7, 8]], columns=['x', 'y'])
df['cat'] = [1, 1, 1, 1]
df.groupby('cat').plot.scatter(x='x', y='y')
```

I get:

![image](https://user-images.githubusercontent.com/26352146/43092541-8e6f4c62-8e7b-11e8-89fc-6991e264e50e.png)

without the changes the output is:

![image](https://user-images.githubusercontent.com/26352146/43092900-96f75ba8-8e7c-11e8-81b2-b15d1220c99e.png)

One Appveyor job keeps getting canceled  without a log. I see other tasks there with similar status. Does anyone know if this is a server issue or from my code?
I don't think we need pytest-mpl.

I'm a bit unsure about the approach here. I'd prefer to see a proper API for groupby that avoids the need to infer the output type, which the groupby plots can use.
@TomAugspurger Do you mean to skip `apply_frame_axis0 ` (or `fast_apply`) and instead take the slow path and loop through the groups when plots are passed as function? 
I'm not sure exactly what the API would be, but the goal is to give callers the ability to disable the "apply called twice" behavior. But this is a larger issue.

Since these groupby methods are internal, we have the ability to hardcode which functions aren't called twice. We do this already for things like `.hist`, `.boxplot`.

The following diff shows an example.

```diff
diff --git a/pandas/core/groupby/base.py b/pandas/core/groupby/base.py
index b2c5a8cff..65e6bdf35 100644
--- a/pandas/core/groupby/base.py
+++ b/pandas/core/groupby/base.py
@@ -59,6 +59,7 @@ class GroupByMixin(object):
 # special case to prevent duplicate plots when catching exceptions when
 # forwarding methods from NDFrames
 plotting_methods = frozenset(['plot', 'boxplot', 'hist'])
+extra_plotting_methods = frozenset(['scatter'])
 
 common_apply_whitelist = frozenset([
     'last', 'first',
diff --git a/pandas/core/groupby/groupby.py b/pandas/core/groupby/groupby.py
index 4b0143b3e..de59673dc 100644
--- a/pandas/core/groupby/groupby.py
+++ b/pandas/core/groupby/groupby.py
@@ -311,6 +311,7 @@ class GroupByPlot(PandasObject):
         def attr(*args, **kwargs):
             def f(self):
                 return getattr(self.plot, name)(*args, **kwargs)
+            f.__name__ = name
             return self._groupby.apply(f)
         return attr
 
diff --git a/pandas/core/groupby/ops.py b/pandas/core/groupby/ops.py
index f2c55a56b..1ba80cbf9 100644
--- a/pandas/core/groupby/ops.py
+++ b/pandas/core/groupby/ops.py
@@ -177,6 +177,7 @@ class BaseGrouper(object):
         # oh boy
         f_name = com._get_callable_name(f)
         if (f_name not in base.plotting_methods and
+                f_name not in base.extra_plotting_methods and
                 hasattr(splitter, 'fast_apply') and axis == 0):
             try:
                 values, mutated = splitter.fast_apply(f, group_keys)
```

Not claiming that's the perfect solution, but I think it's a bit cleaner than changing `apply_frame_axis0`
Thanks, didn't realize this was already there
@TomAugspurger Since you already have this written up do you want to push your version?
FYI, you can apply diffs with `git apply`

```python
git apply foo.patch
```

You'll want to include other plot methods than scatter, revert the changes to `apply_frame_axis_0`, update the tests, etc.
Ok, thanks.
@TomAugspurger your patch (plus added plot names) fixes all plot functions, when I try:
```python
%matplotlib inline
import matplotlib.pyplot as plt
import pandas as pd
df = pd.DataFrame([[1, 2], [3, 4], [5, 6], [7, 8]], columns=['x', 'y'])
df['cat'] = [1, 1, 1, 1]

extra_plotting_methods = ['line', 'bar', 'barh', 'box', 'kde', 'density', 'area', 'pie', 'scatter', 'hexbin']
p = df.groupby('cat').plot

for plotting_method in extra_plotting_methods:
    ax = p.__getattr__(plotting_method)(x='x', y='y');
    ax.iloc[0].set_title(plotting_method)
```

Still not sure how to implement a test though.
By the way, not sure if .kde and .density plots are meant to ever be used without `x`, `y` arguments. So just putting this out there. Does the following deserve its own issue?:

```python
%matplotlib inline
import matplotlib.pyplot as plt
import pandas as pd

df = pd.DataFrame([[1, 2], [3, 4], [5, 6], [7, 8]], columns=['x', 'y'])
df['cat'] = [1, 1, 1, 1]

df.groupby('cat').plot.kde();
```

![image](https://user-images.githubusercontent.com/26352146/43161109-236d9fce-8f55-11e8-93a4-49d2af54e837.png)

similar behavior for .density
@javadnoorb looks like there are unrelated commits on this PR now, do you need help with git stuff? https://pandas.pydata.org/pandas-docs/stable/contributing.html#working-with-the-code might be helpful as well.
@TomAugspurger yeah. I messed up while syncing my fork. There were too many conflicts after the merge and I manually removed mine against the master, and some newlines were left here and there. Was I supposed to do this manually? 

I think there was a delay in sync that caused this. Sorry about that. I'll clean it up.
@jreback @toobaz @TomAugspurger. Just leaving a note. This should be ready for review. -thanks
@TomAugspurger, I don't think `plotting_methods` and `extra_plotting_methods` should be merged. It leads to some errors like:

`AttributeError: type object 'Series' has no attribute 'hexbin'`

`plotting_methods` affects:

https://github.com/pandas-dev/pandas/blob/26b3e7de56fdebaf1842b01d747a1a30a126c54a/pandas/core/groupby/base.py#L80

and 

https://github.com/pandas-dev/pandas/blob/26b3e7de56fdebaf1842b01d747a1a30a126c54a/pandas/core/groupby/base.py#L86

which are then used as methods for dataframes and series.

I think only functions used in this doc can be methods of `groupby.plot`:

http://pandas.pydata.org/pandas-docs/version/0.16.2/generated/pandas.core.groupby.DataFrameGroupBy.plot.html

`hist` is the only exception because it has dual functionality (both for ~~series/dataframes~~ `groupby` and `groupby.plot`). But I think it should be sufficient to have it in `plotting_methods`.

Following the other point, adding `extra_plotting_methods` to core/goupby/groupby.py:`_make_wrapper` doesn't seem to do anything when I try it. These `curried_with_axis` functions are pretty old and have no issue number. I can't track down why they were written, but seems like they just explicitly add an axis to the plot if none is provided. 
Thanks for the clarification on merging those two sets. Is the make_wrapper code dead? Does removing it break any tests? I’ve never touched them.

________________________________
From: Javad Noorbakhsh <notifications@github.com>
Sent: Monday, July 30, 2018 2:25:56 PM
To: pandas-dev/pandas
Cc: Tom Augspurger; Mention
Subject: Re: [pandas-dev/pandas] apply_frame_axis0 applies function to first group twice (#21943)


@TomAugspurger<https://github.com/TomAugspurger>, I don't think plotting_methods and extra_plotting_methods should be merged. It leads to some errors like:

AttributeError: type object 'Series' has no attribute 'hexbin'

plotting_methods affects:

https://github.com/pandas-dev/pandas/blob/26b3e7de56fdebaf1842b01d747a1a30a126c54a/pandas/core/groupby/base.py#L80

and

https://github.com/pandas-dev/pandas/blob/26b3e7de56fdebaf1842b01d747a1a30a126c54a/pandas/core/groupby/base.py#L86

which are then used as methods for dataframes and series.

I think only functions used in this doc can be methods of groupby.plot:

http://pandas.pydata.org/pandas-docs/version/0.16.2/generated/pandas.core.groupby.DataFrameGroupBy.plot.html

hist is the only exception because it has dual functionality (both for series/dataframes and groups). But I think it should be sufficient to have it in plotting_methods.

Following the other point, adding extra_plotting_methods to core/goupby/groupby.py:_make_wrapper doesn't seem to do anything when I try it. These curried_with_axis functions are pretty old and have no issue number. I can't track down why they were written, but seems like they just explicitly add an axis to the plot if none is provided.

—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub<https://github.com/pandas-dev/pandas/pull/21943#issuecomment-408981161>, or mute the thread<https://github.com/notifications/unsubscribe-auth/ABQHIlKw6qDVSnqrNnc8dHZOliHPC3Cyks5uL13EgaJpZM4VSJaX>.