Repository: dask/dask
Pull request: fix groupby.std() with integer colum names
- [X] Tests added / passed
- [ ] Passes `black dask` / `flake8 dask`

Fixes #3560

I tried using the tuples solution proposed in https://github.com/dask/dask/issues/3560#issuecomment-395842847 but pandas wasn't happy.

Note that the solution in this PR might cause collisions if we have a column 0 and another column '0'.

Problem statement:
Standard Deviation error after GroupBy, Dask DataFrame
**Description**
I bumped into a case where, after GroupBy's of two Dask DataFrames, I can calculate the *sum* and *mean* but not *std*.

**Steps to reproduce**
```py
import numpy as np
import dask.array as da
import dask.dataframe as dd

# Data
data_array = da.from_array(np.array(range(1,37)).reshape((6,6)), chunks=-1)

# For grouping
df = da.from_array(np.array([0,2,0,2,0,2]), chunks=-1).to_dask_dataframe()
```
Now 
```py
data_array.to_dask_dataframe().groupby(df).sum()
# and
data_array.to_dask_dataframe().groupby(df).mean()
```
are possible to calculate, but
```py
data_array.to_dask_dataframe().groupby(df).std()
```
is not. It gives following errors:

```py
TypeError: unsupported operand type(s) for +: 'int' and 'str'
```
and
```py
ValueError: Metadata inference failed in `_var_chunk`.

You have supplied a custom function and Dask is unable to 
determine the type of output that that function returns. 

To resolve this please provide a meta= keyword.
The docstring of the Dask function you ran should have more information.

Original error is below:
------------------------
TypeError("unsupported operand type(s) for +: 'int' and 'str'",)
```

**Own guess**
My first guess would be that this has something to do with chunks/chunksizes but I am not sure.

**Versions**
* Linux 4.13.0-43-generic x86_64
* Python 3.5.2
* Dask 0.17.5
* NumPy 1.14.3

Hints from issue discussion:
Thanks for reporting this issue and providing an example to reproduce the problem @JukkaKeisala! It looks like the issue might be arising here 

https://github.com/dask/dask/blob/e1c48e0c970aeb81ffeb58791db2ca3ce76fa846/dask/dataframe/groupby.py#L243-L244

where `c + '-x2'` and `c + '-count'` are raising the `TypeError` you're seeing because the columns of the Dataframe in your example are integers. 

For example, after modifying your above example slightly to make sure the columns are strings instead of integers, no error is raised. 

<details>
<summary> Example code: </summary>

```python
import numpy as np
import dask.array as da
import dask.dataframe as dd

# Data
data_array = da.from_array(np.array(range(1,37)).reshape((6,6)), chunks=-1)

# For grouping
df = da.from_array(np.array([0,2,0,2,0,2]), chunks=-1).to_dask_dataframe()

columns = list(map(str, np.arange(data_array.shape[1])))
gb = data_array.to_dask_dataframe(columns=columns).groupby(df)
print('sum:\n{}'.format(gb.sum().compute()))
print('mean:\n{}'.format(gb.mean().compute()))
print('std:\n{}'.format(gb.std().compute()))
```

</details>
<br>
<details>
<summary>Example output:</summary>
 
```
sum:
    0   1   2   3   4   5
0  39  42  45  48  51  54
2  57  60  63  66  69  72
mean:
      0     1     2     3     4     5
0  13.0  14.0  15.0  16.0  17.0  18.0
2  19.0  20.0  21.0  22.0  23.0  24.0
std:
      0     1     2     3     4     5
0  12.0  12.0  12.0  12.0  12.0  12.0
2  12.0  12.0  12.0  12.0  12.0  12.0
```
</details>

<br>

This is definitely something that should be resolved. @JukkaKeisala are you interested in contributing here? 
Thank you @jrbourbeau for pointing out a solution. I am not sure if my know-how is enough to solve this. 

I looked at the code and followed it to dask/dataframe/core.py where the function called `apply_concat_apply` (a.k.a. `aca`) handles our DataFrame, that is returned from `_var_chunk`. `aca` must somehow handle and dismantle the columns by their names and therefore they are renamed as strings... On the other hand, the number of columns of  `pd.concat([x, x2, n], axis=1) is divisible to three equal sized parts so that information does not need to be input in columns names.

Quick fix would be to replace `...rename(columns=lambda c: c + '-x2')` by `...rename(columns=lambda c: str(c) + '-x2')` and raise a warning to remember the user that integer type columns are changed to str-type columns until this is properly fixed. But I don't think if this is a proper way to do things.
> On the other hand, the number of columns of `pd.concat([x, x2, n], axis=1)` is divisible to three equal sized parts so that information does not need to be input in columns names.

I think that avoiding the column rename as you suggest here is a great potential solution to this problem! That would avoid making any assumptions about the column `dtype`. The main downside of removing the rename is we would loose the benefit of clearly knowing which columns are which by looking at the column names (i.e. currently we know the squared columns are the ones with `-x2`, etc.). But that might be a good trade-off. 

@mrocklin any thoughts on the topic of removing the `rename` from lines like 

https://github.com/dask/dask/blob/e1c48e0c970aeb81ffeb58791db2ca3ce76fa846/dask/dataframe/groupby.py#L243-L244

in `_var_chunk` and `_var_agg` in `dask/dataframe/groupby.py`? 


> I am not sure if my know-how is enough to solve this.

Based on your explanation of the problem and proposed solution, I'd say your know-how is enough :) 
I would like to contribute on this, if there is a common understanding about removing `rename`s and handling the consequences. @jrbourbeau, @mrocklin?
Sorry for the lack of response here.  Thank you @JukkaKeisala for raising the issue and @jrbourbeau for diagnosing it.  

Removing the renaming sounds nicer than erring.  Would it be possible to rename instead to a tuple or list name?

    rename(lambda c: (c, 'count'))

> Would it be possible to rename instead to a tuple or list name?

That's totally possible and would be a nice way to keep the additional `'x2'` and `'count'` information explicit while allowing for more general column `dtype`s 👍 
That sounds very good!

I'll start working on this.


> Would it be possible to rename instead to a tuple or list name?
>
> That's totally possible and would be a nice way to keep the additional
> 'x2' and 'count' information explicit while allowing for more general
> column dtypes 👍


Hey @JukkaKeisala, just wanted to check in to see how things are going with this issue. If you run into any problems, feel free to ask me about them.
Hi and thanks for asking. For a couple of reasons, I have not found time on this, But this is still on my to-do-list.
Not a problem, just wanted to check in. Thanks again for working on this @JukkaKeisala 👍 !

Commit message: used tuples

Patch to review:
diff --git a/dask/dataframe/groupby.py b/dask/dataframe/groupby.py
index b0c9e42ac93..894cd6bba08 100644
--- a/dask/dataframe/groupby.py
+++ b/dask/dataframe/groupby.py
@@ -284,11 +284,11 @@ def _var_chunk(df, *index):
     g = _groupby_raise_unaligned(df, by=index)
     x = g.sum()
 
-    n = g[x.columns].count().rename(columns=lambda c: c + "-count")
+    n = g[x.columns].count().rename(columns=lambda c: (c, "-count"))
 
     df[cols] = df[cols] ** 2
     g2 = _groupby_raise_unaligned(df, by=index)
-    x2 = g2.sum().rename(columns=lambda c: c + "-x2")
+    x2 = g2.sum().rename(columns=lambda c: (c, "-x2"))
 
     x2.index = x.index
     return concat([x, x2, n], axis=1)
@@ -302,8 +302,9 @@ def _var_agg(g, levels, ddof):
     g = g.groupby(level=levels, sort=False).sum()
     nc = len(g.columns)
     x = g[g.columns[: nc // 3]]
-    x2 = g[g.columns[nc // 3 : 2 * nc // 3]].rename(columns=lambda c: c[:-3])
-    n = g[g.columns[-nc // 3 :]].rename(columns=lambda c: c[:-6])
+    # chunks columns are tuples (number, str), so we just keep the number part
+    x2 = g[g.columns[nc // 3 : 2 * nc // 3]].rename(columns=lambda c: c[0])
+    n = g[g.columns[-nc // 3 :]].rename(columns=lambda c: c[0])
 
     # TODO: replace with _finalize_var?
     result = x2 - x ** 2 / n
diff --git a/dask/dataframe/tests/test_groupby.py b/dask/dataframe/tests/test_groupby.py
index c8cd35585dc..7ce9e6aff59 100644
--- a/dask/dataframe/tests/test_groupby.py
+++ b/dask/dataframe/tests/test_groupby.py
@@ -1883,6 +1883,16 @@ def test_std_object_dtype(func):
     assert_eq(func(df), func(ddf))
 
 
+def test_std_columns_int():
+    # Make sure std() works when index_by is a df with integer column names
+    # Non regression test for issue #3560
+
+    df = pd.DataFrame({0: [5], 1: [5]})
+    ddf = dd.from_pandas(df, npartitions=2)
+    by = dask.array.from_array([0, 1]).to_dask_dataframe()
+    ddf.groupby(by).std()
+
+
 def test_timeseries():
     df = dask.datasets.timeseries().partitions[:2]
     assert_eq(df.groupby("name").std(), df.groupby("name").std())


Code review comments:
diff_hunk: @@ -302,8 +302,9 @@ def _var_agg(g, levels, ddof):
     g = g.groupby(level=levels, sort=False).sum()
     nc = len(g.columns)
     x = g[g.columns[: nc // 3]]
-    x2 = g[g.columns[nc // 3 : 2 * nc // 3]].rename(columns=lambda c: c[:-3])
-    n = g[g.columns[-nc // 3 :]].rename(columns=lambda c: c[:-6])
+    # chunks columns are tuples (number, str), so we just keep the number part
path: dask/dataframe/groupby.py
text: @user1:
Small nitpick: in general `c` can be non-numeric (e.g. string columns)

```suggestion
    # chunks columns are tuples (value, name), so we just keep the value part
```