BUG: Fix #10355, std() groupby calculation
State: closed

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


Labels:
name: Bug
name: Groupby

Comments:
# [Codecov](https://codecov.io/gh/pandas-dev/pandas/pull/26229?src=pr&el=h1) Report
> Merging [#26229](https://codecov.io/gh/pandas-dev/pandas/pull/26229?src=pr&el=desc) into [master](https://codecov.io/gh/pandas-dev/pandas/commit/64104ec343f0770afa88f02da47b34fbe62c8281?src=pr&el=desc) will **decrease** coverage by `51.27%`.
> The diff coverage is `0%`.

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

```diff
@@             Coverage Diff             @@
##           master   #26229       +/-   ##
===========================================
- Coverage   91.97%    40.7%   -51.28%     
===========================================
  Files         175      175               
  Lines       52379    52380        +1     
===========================================
- Hits        48178    21321    -26857     
- Misses       4201    31059    +26858
```

| Flag | Coverage Δ | |
|---|---|---|
| #multiple | `?` | |
| #single | `40.7% <0%> (-0.15%)` | :arrow_down: |

| [Impacted Files](https://codecov.io/gh/pandas-dev/pandas/pull/26229?src=pr&el=tree) | Coverage Δ | |
|---|---|---|
| [pandas/core/groupby/groupby.py](https://codecov.io/gh/pandas-dev/pandas/pull/26229/diff?src=pr&el=tree#diff-cGFuZGFzL2NvcmUvZ3JvdXBieS9ncm91cGJ5LnB5) | `24.45% <0%> (-72.78%)` | :arrow_down: |
| [pandas/io/formats/latex.py](https://codecov.io/gh/pandas-dev/pandas/pull/26229/diff?src=pr&el=tree#diff-cGFuZGFzL2lvL2Zvcm1hdHMvbGF0ZXgucHk=) | `0% <0%> (-100%)` | :arrow_down: |
| [pandas/io/sas/sas\_constants.py](https://codecov.io/gh/pandas-dev/pandas/pull/26229/diff?src=pr&el=tree#diff-cGFuZGFzL2lvL3Nhcy9zYXNfY29uc3RhbnRzLnB5) | `0% <0%> (-100%)` | :arrow_down: |
| [pandas/core/groupby/categorical.py](https://codecov.io/gh/pandas-dev/pandas/pull/26229/diff?src=pr&el=tree#diff-cGFuZGFzL2NvcmUvZ3JvdXBieS9jYXRlZ29yaWNhbC5weQ==) | `0% <0%> (-100%)` | :arrow_down: |
| [pandas/tseries/plotting.py](https://codecov.io/gh/pandas-dev/pandas/pull/26229/diff?src=pr&el=tree#diff-cGFuZGFzL3RzZXJpZXMvcGxvdHRpbmcucHk=) | `0% <0%> (-100%)` | :arrow_down: |
| [pandas/tseries/converter.py](https://codecov.io/gh/pandas-dev/pandas/pull/26229/diff?src=pr&el=tree#diff-cGFuZGFzL3RzZXJpZXMvY29udmVydGVyLnB5) | `0% <0%> (-100%)` | :arrow_down: |
| [pandas/io/formats/html.py](https://codecov.io/gh/pandas-dev/pandas/pull/26229/diff?src=pr&el=tree#diff-cGFuZGFzL2lvL2Zvcm1hdHMvaHRtbC5weQ==) | `0% <0%> (-99.37%)` | :arrow_down: |
| [pandas/io/sas/sas7bdat.py](https://codecov.io/gh/pandas-dev/pandas/pull/26229/diff?src=pr&el=tree#diff-cGFuZGFzL2lvL3Nhcy9zYXM3YmRhdC5weQ==) | `0% <0%> (-91.16%)` | :arrow_down: |
| [pandas/io/sas/sas\_xport.py](https://codecov.io/gh/pandas-dev/pandas/pull/26229/diff?src=pr&el=tree#diff-cGFuZGFzL2lvL3Nhcy9zYXNfeHBvcnQucHk=) | `0% <0%> (-90.1%)` | :arrow_down: |
| [pandas/core/tools/numeric.py](https://codecov.io/gh/pandas-dev/pandas/pull/26229/diff?src=pr&el=tree#diff-cGFuZGFzL2NvcmUvdG9vbHMvbnVtZXJpYy5weQ==) | `10.44% <0%> (-89.56%)` | :arrow_down: |
| ... and [131 more](https://codecov.io/gh/pandas-dev/pandas/pull/26229/diff?src=pr&el=tree-more) | |

------

[Continue to review full report at Codecov](https://codecov.io/gh/pandas-dev/pandas/pull/26229?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/26229?src=pr&el=footer). Last update [64104ec...493901e](https://codecov.io/gh/pandas-dev/pandas/pull/26229?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/26229?src=pr&el=h1) Report
> Merging [#26229](https://codecov.io/gh/pandas-dev/pandas/pull/26229?src=pr&el=desc) into [master](https://codecov.io/gh/pandas-dev/pandas/commit/1263e1a9cac179f453aa6223f00cbaa3e158a878?src=pr&el=desc) will **decrease** coverage by `50.05%`.
> The diff coverage is `0%`.

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

```diff
@@             Coverage Diff             @@
##           master   #26229       +/-   ##
===========================================
- Coverage   91.73%   41.68%   -50.06%     
===========================================
  Files         174      174               
  Lines       50741    50746        +5     
===========================================
- Hits        46548    21154    -25394     
- Misses       4193    29592    +25399
```

| Flag | Coverage Δ | |
|---|---|---|
| #multiple | `?` | |
| #single | `41.68% <0%> (-0.11%)` | :arrow_down: |

| [Impacted Files](https://codecov.io/gh/pandas-dev/pandas/pull/26229?src=pr&el=tree) | Coverage Δ | |
|---|---|---|
| [pandas/core/groupby/ops.py](https://codecov.io/gh/pandas-dev/pandas/pull/26229/diff?src=pr&el=tree#diff-cGFuZGFzL2NvcmUvZ3JvdXBieS9vcHMucHk=) | `19.47% <ø> (-76.49%)` | :arrow_down: |
| [pandas/core/groupby/groupby.py](https://codecov.io/gh/pandas-dev/pandas/pull/26229/diff?src=pr&el=tree#diff-cGFuZGFzL2NvcmUvZ3JvdXBieS9ncm91cGJ5LnB5) | `24.24% <0%> (-73%)` | :arrow_down: |
| [pandas/io/formats/latex.py](https://codecov.io/gh/pandas-dev/pandas/pull/26229/diff?src=pr&el=tree#diff-cGFuZGFzL2lvL2Zvcm1hdHMvbGF0ZXgucHk=) | `0% <0%> (-100%)` | :arrow_down: |
| [pandas/io/sas/sas\_constants.py](https://codecov.io/gh/pandas-dev/pandas/pull/26229/diff?src=pr&el=tree#diff-cGFuZGFzL2lvL3Nhcy9zYXNfY29uc3RhbnRzLnB5) | `0% <0%> (-100%)` | :arrow_down: |
| [pandas/core/groupby/categorical.py](https://codecov.io/gh/pandas-dev/pandas/pull/26229/diff?src=pr&el=tree#diff-cGFuZGFzL2NvcmUvZ3JvdXBieS9jYXRlZ29yaWNhbC5weQ==) | `0% <0%> (-100%)` | :arrow_down: |
| [pandas/tseries/plotting.py](https://codecov.io/gh/pandas-dev/pandas/pull/26229/diff?src=pr&el=tree#diff-cGFuZGFzL3RzZXJpZXMvcGxvdHRpbmcucHk=) | `0% <0%> (-100%)` | :arrow_down: |
| [pandas/tseries/converter.py](https://codecov.io/gh/pandas-dev/pandas/pull/26229/diff?src=pr&el=tree#diff-cGFuZGFzL3RzZXJpZXMvY29udmVydGVyLnB5) | `0% <0%> (-100%)` | :arrow_down: |
| [pandas/io/formats/html.py](https://codecov.io/gh/pandas-dev/pandas/pull/26229/diff?src=pr&el=tree#diff-cGFuZGFzL2lvL2Zvcm1hdHMvaHRtbC5weQ==) | `0% <0%> (-99.37%)` | :arrow_down: |
| [pandas/io/sas/sas7bdat.py](https://codecov.io/gh/pandas-dev/pandas/pull/26229/diff?src=pr&el=tree#diff-cGFuZGFzL2lvL3Nhcy9zYXM3YmRhdC5weQ==) | `0% <0%> (-91.16%)` | :arrow_down: |
| [pandas/io/sas/sas\_xport.py](https://codecov.io/gh/pandas-dev/pandas/pull/26229/diff?src=pr&el=tree#diff-cGFuZGFzL2lvL3Nhcy9zYXNfeHBvcnQucHk=) | `0% <0%> (-90.1%)` | :arrow_down: |
| ... and [131 more](https://codecov.io/gh/pandas-dev/pandas/pull/26229/diff?src=pr&el=tree-more) | |

------

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

did the patch i put up in the original issue not solve this?


Hello @alexcwatt! 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-05-17 13:55:48 UTC
Thaks!
can you merge master and update
Merged master and made some updates to the unit tests. I am now looking into some failing test cases (`pytest pandas\tests\resample\ -k test_resample_empty_series`), will try to figure out this weekend but wanted to push the other minor updates.
Still have a bit more to do with this one corner case. I think the reason it hasn't been an issue too much in the past is that some methods like groupby's mean just catch all exceptions coming from the cython_agg_general and fall back on Python. I can do that if I have to, and revert my changes to reduction.pyx, but I am trying to grok what's going on in there so we don't need to catch a general Exception and fall back on Python.

If anyone can shed some light please feel free, otherwise again I am going to try to understand and resolve it this weekend.
thanks for working on this @alexcwatt groupby is a bit tricky :-D
@jreback Can you please confirm that the test that is failing does specify the desired behavior? Currently the expectation is that it should return an empty dataframe, with no columns. The actual result is now that it returns an empty dataframe, with a column '0'. The column '0' is seen in other tests like `test_resample_with_nat` that actually return data but the test `test_resample_with_only_nat` is written to specify that we expect an empty dataframe with no columns...
@alexcwatt I wasn't able to reproduce that issue off of this locally; restarted CI to see
Ignore previous comment I was able to reproduce issue after building extensions. So yea @alexcwatt it looks like these changes are implicitly adding a blank column to that test which we wouldn't want
Okay, thanks for the confirmation, will look into it when I can

On Mon, May 20, 2019, at 12:46 PM, William Ayd wrote:
> Ignore previous comment I was able to reproduce issue after building extensions. So yea @alexcwatt <https://github.com/alexcwatt> it looks like these changes are implicitly adding a blank column to that test which we wouldn't want

> —
> You are receiving this because you were mentioned.
> Reply to this email directly, view it on GitHub <https://github.com/pandas-dev/pandas/pull/26229?email_source=notifications&email_token=AADYU6NIYF2SQ24WKQFLB6TPWLIXDA5CNFSM4HI5FVL2YY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGODVZNNOQ#issuecomment-494065338>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AADYU6JE2DS3LA3D2LYPKYTPWLIXDANCNFSM4HI5FVLQ>.

can you merge master and see if you can get passing
can you merge master.
I'll take another look tonight. Unfortunately what I've found is that there is an exception being raised by the Cython code that I don't quite understand, and the way some of the other groupby operations work is that they catch exceptions from Cython and fall back to the Python version that doesn't have the bug/issue. I think this is what I need to do for std() as well.

Edit: When I tried fixing this in the Cython it lead to that one unit test failing, and I haven't been able to figure out how to prevent that purely Cython side. Might open a bug about the Cython code to be fixed later, with notes on the direction I was going.
Pushing to 0.25.1. @Alex if you merge master sometime later today there will be a 0.25.1.rst for the release note.
Nice PR but I think this has gone stale so closing. ping if you'd like to pick back up