-
[ x ] I have checked that this issue has not already been reported.
-
[ x ] I have confirmed this bug exists on the latest version of pandas.
-
[ ] (optional) I have confirmed this bug exists on the master branch of pandas.
Note: Please read this guide detailing how to provide the necessary information for us to reproduce your bug.
Code Sample, a copy-pastable example
import pandas as pd
for i in range(100):
n = 2 ** i
print(f"Trying n = 2 ** {i} '{n}'...")
df = pd.DataFrame([['A', 14], ['A', n]], columns=['gb', 'val'])
gb_sum = df.groupby('gb').sum().values[0][0]
df_sum = df.sum().values[1]
if gb_sum != df_sum:
print(f"df.sum().values[1] '{df_sum}' != df.groupby('gb').sum().values[0][0] '{gb_sum}")
break
del df
print(f"Runing pandas {pd.__version__}")
Problem description
groupby.sum() results currently provide different results for df.sum() results for large integers.
It is expected that they should provide the same results.
Expected Output
When grouping by a colum with a single value, the groupby().sum() result should always equal the df.sum() result()
Output of pd.show_versions()
Comment From: TomAugspurger
Thanks for the report. There's a cast to float64
> /Users/taugspurger/sandbox/pandas/pandas/core/groupby/generic.py(1043)_cython_agg_blocks()
-> try:
(Pdb) l
1038 for block in data.blocks:
1039 # Avoid inheriting result from earlier in the loop
1040 result = no_result
1041 locs = block.mgr_locs.as_array
1042 breakpoint()
1043 -> try:
1044 result, _ = self.grouper.aggregate(
1045 block.values, how, axis=1, min_count=min_count
1046 )
1047 except NotImplementedError:
1048 # generally if we have numeric_only=False
(Pdb) block.values
array([[ 14, 18014398509481984]])
(Pdb) how
'add'
(Pdb) n
> /Users/taugspurger/sandbox/pandas/pandas/core/groupby/generic.py(1044)_cython_agg_blocks()
-> result, _ = self.grouper.aggregate(
(Pdb) n
> /Users/taugspurger/sandbox/pandas/pandas/core/groupby/generic.py(1045)_cython_agg_blocks()
-> block.values, how, axis=1, min_count=min_count
(Pdb) n
> /Users/taugspurger/sandbox/pandas/pandas/core/groupby/generic.py(1091)_cython_agg_blocks()
-> assert not isinstance(result, DataFrame)
(Pdb) result
array([[1.80143985e+16]])
(Pdb) result.dtype
dtype('float64')
I think i occurs somewhere in groupby/ops.py
> /Users/taugspurger/sandbox/pandas/pandas/core/groupby/ops.py(519)_cython_operation()
-> func, values = self._get_cython_func_and_vals(kind, how, values, is_numeric)
@tom-weiss are you interested in debugging any further?
Comment From: TomAugspurger
Oh, perhaps it's as simple as us just not having an int64-dtype group_add function in libgroupby. Does that sound right @WillAyd?
Comment From: WillAyd
At first glance seems logical; not sure why it's not there to begin with
Comment From: tom-weiss
I am happy to help debug and assist as I can but am entirely unfamiliar with the inner workings of the pandas codebase.
Comment From: WillAyd
The line of interest is here:
https://github.com/pandas-dev/pandas/blob/6efb0b20ae13c3daaf086631e85491b81dca171a/pandas/_libs/groupby.pyx#L440
I don't know the history as to why only complex / floating are listed here. This may be better served just using Cython.numeric instead of a custom fused type
In any case if you wanted to try, I would suggest adding a test for the issue in pandas/tests/groupby/aggregate/test_aggregate.py and changing the lines above as described to see if you can get it to work
Comment From: tom-weiss
So the issue appears to be how to handle NAN values. It's easy enough to switch to a numeric but then I guess issues on line 482:
if nobs[i, j] < min_count:
out[i, j] = NAN
I can't assign type 'long double' to 'uint64_t'
Comment From: WillAyd
You can qualify the preceding condition with something like if numeric is not cython.integral_t to prevent assigning NA values to integer types
Comment From: WillAyd
See http://docs.cython.org/en/latest/src/userguide/fusedtypes.html#type-checking-specializations
Comment From: tom-weiss
So it's quite easy to fix this to work with Integers but the required pandas functionality with max_count in the groupby() method is to return NaN for values that don't reach min_count(): https://pandas.pydata.org/pandas-docs/stable/reference/api/pandas.DataFrame.sum.html
Does anyone know how I can return the NAN in the case where I want to do integer arithmetic?
Comment From: jreback
we always cast to float64 because sum can overflow int64 with large inputs ; we do try to cast back if possible i think
Comment From: tom-weiss
@jreback yes I can see the results cast back to int64 but we're getting rounding errors that are throwing the results out. These are not present in the standard df.sum() path.
Comment From: jreback
the soln of course is to add a groupby int64 sum kernel
Comment From: tom-weiss
Is there any easy way to make int64 nullable in cython?
The _groupby_add function in pandas/_libs/groupby.pyx uses numpy types which are not-nullable for integers.
I'm not clear how I could implement the new nullable integer type into this: https://pandas.pydata.org/pandas-docs/stable/user_guide/integer_na.html.
My curent code is here:
ctypedef fused complexfloating_t:
int16_t
int32_t
int64_t
int8_t
uint16_t
uint32_t
uint64_t
uint8_t
float64_t
float32_t
complex64_t
complex128_t
@cython.wraparound(False)
@cython.boundscheck(False)
def _group_add(complexfloating_t[:, :] out,
int64_t[:] counts,
complexfloating_t[:, :] values,
const int64_t[:] labels,
Py_ssize_t min_count=0):
"""
Only aggregates on axis=0
"""
cdef:
Py_ssize_t i, j, N, K, lab, ncounts = len(counts)
complexfloating_t val, count
complexfloating_t[:, :] sumx
int64_t[:, :] nobs
if len(values) != len(labels):
raise ValueError("len(index) != len(labels)")
nobs = np.zeros((<object>out).shape, dtype=np.int64)
sumx = np.zeros_like(out)
N, K = (<object>values).shape
with nogil:
for i in range(N):
lab = labels[i]
if lab < 0:
continue
counts[lab] += 1
for j in range(K):
val = values[i, j]
# not nan
if val == val:
nobs[lab, j] += 1
if (complexfloating_t is complex64_t or
complexfloating_t is complex128_t):
# clang errors if we use += with these dtypes
sumx[lab, j] = sumx[lab, j] + val
else:
sumx[lab, j] += val
for i in range(ncounts):
for j in range(K):
if nobs[i, j] < min_count:
if (complexfloating_t is int8_t or
complexfloating_t is int16_t or
complexfloating_t is int32_t or
complexfloating_t is int64_t or
complexfloating_t is uint64_t or
complexfloating_t is uint32_t or
complexfloating_t is uint16_t or
complexfloating_t is uint8_t):
out[i, j] = 0 # this needs to be pd.NA. How to do?
else:
out[i, j] = NAN
else:
out[i, j] = sumx[i, j]
group_add_int8 = _group_add['int8_t']
group_add_int16 = _group_add['int16_t']
group_add_int32 = _group_add['int32_t']
group_add_int64 = _group_add['int64_t']
group_add_uint64 = _group_add['uint64_t']
group_add_uint32 = _group_add['uint32_t']
group_add_uint16 = _group_add['uint16_t']
group_add_uint8 = _group_add['uint8_t']
group_add_float32 = _group_add['float32_t']
group_add_float64 = _group_add['float64_t']
group_add_complex64 = _group_add['float complex']
group_add_complex128 = _group_add['double complex']
Comment From: jreback
nullables are not supported directly in cython, this can be worked around by actually passing the mask to the cython functions, this is will require some reworking of the code and calling conventions.
Comment From: TomAugspurger
For this specific case (int64), we might not need to worry about min_count in the cython level. That can be done outside of it, since we know that the number of valid values equals the length of the array.
However a masked groupby sum would be helpful for the nullable integer dtype.
Comment From: tom-weiss
If someone can point me in the right direction I am happy to try and fix or help validate a fix.
Comment From: rhshadrach
OP example now has gb_sum always equal to df_sum. Would be good to run a git bisect to see which PR fixed this and if this needs tests.
Comment From: deejlucas
take
Comment From: deejlucas
Update: It's been slow going as it took me a while to build an environment where I could perform git bisect for versions 1.x.
I have results from the bisect now that say that the bug has been fixed between d3f08566a80239a18a813ebda9a2ebb0368b1dc5 and 1ce1c3c1ef9894bf1ba79805f37514291f52a9da. I will double check that result, write some tests, and hopefully submit a PR soon.
Comment From: jahn96
@deejlucas happy to take on this issue if you aren't working on it
Comment From: jahn96
take