BUG: cython groupby cummin/cummax assumes groups ordered

[adbull]

Code Sample, a copy-pastable example if possible

>>> import pandas as pd
>>> pd.DataFrame(dict(a=[1,2,1], b=[2,1,1])).groupby('a').b.cummax()
0    2
1    1
2    1
Name: b, dtype: int64

Problem description

The current github version of pandas has cython implementations of groupby.cummin and groupby.cummax which give the wrong answer when the groups are unordered. (See #15048, 0fe491d.)

Expected Output

0    2
1    1
2    2
Name: b, dtype: int64

Output of pd.show_versions()

INSTALLED VERSIONS

commit: None
python: 3.5.2.final.0
python-bits: 64
OS: Linux
OS-release: 4.9.8-100.fc24.x86_64
machine: x86_64
processor: x86_64
byteorder: little
LC_ALL: C
LANG: C
LOCALE: None.None

pandas: 0.19.0+575.g5667a3a
pytest: 3.0.5
pip: 9.0.1
setuptools: 27.2.0
Cython: 0.25.2
numpy: 1.11.3
scipy: 0.18.1
xarray: 0.9.1
IPython: 4.2.0
sphinx: 1.5.1
patsy: 0.4.1
dateutil: 2.6.0
pytz: 2016.10
blosc: None
bottleneck: 1.2.0
tables: 3.3.0
numexpr: 2.6.2
feather: None
matplotlib: 2.0.0
openpyxl: 2.4.1
xlrd: 1.0.0
xlwt: 1.2.0
xlsxwriter: 0.9.6
lxml: 3.7.2
bs4: 4.5.3
html5lib: 0.999
sqlalchemy: 1.1.5
pymysql: None
psycopg2: None
jinja2: 2.9.4
s3fs: None
pandas_gbq: None
pandas_datareader: None

[jreback]

looks like a bug in how the maxval is updated

it's not about the group ordering or not but a straight up bug

pr to fix is welcome

[jreback]

cc @mroeschke

[adbull]

Ah, agreed. I think it happens to get the right answer on ordered groups, which is why the tests passed.

Suspect it just requires the following, plus tests of course?

diff --git a/pandas/_libs/algos_groupby_helper.pxi.in b/pandas/_libs/algos_groupby_helper.pxi.in
index 9552b42..c86a4b4 100644
--- a/pandas/_libs/algos_groupby_helper.pxi.in
+++ b/pandas/_libs/algos_groupby_helper.pxi.in
@@ -603,7 +603,7 @@ def group_cummin_{{name}}(ndarray[{{dest_type2}}, ndim=2] out,
     """
     cdef:
         Py_ssize_t i, j, N, K, size
-        {{dest_type2}} val, min_val = 0
+        {{dest_type2}} val
         ndarray[{{dest_type2}}, ndim=2] accum
         int64_t lab
 
@@ -629,8 +629,7 @@ def group_cummin_{{name}}(ndarray[{{dest_type2}}, ndim=2] out,
                 if val == val:
                 {{endif}}
                     if val < accum[lab, j]:
-                        min_val = val
-                    accum[lab, j] = min_val
+                        accum[lab, j] = val
                     out[i, j] = accum[lab, j]
 
 
@@ -645,7 +644,7 @@ def group_cummax_{{name}}(ndarray[{{dest_type2}}, ndim=2] out,
     """
     cdef:
         Py_ssize_t i, j, N, K, size
-        {{dest_type2}} val, max_val = 0
+        {{dest_type2}} val
         ndarray[{{dest_type2}}, ndim=2] accum
         int64_t lab
 
@@ -670,8 +669,7 @@ def group_cummax_{{name}}(ndarray[{{dest_type2}}, ndim=2] out,
                 if val == val:
                 {{endif}}
                     if val > accum[lab, j]:
-                        max_val = val
-                    accum[lab, j] = max_val
+                        accum[lab, j] = val
                     out[i, j] = accum[lab, j]
 
 {{endfor}}

[jreback]

yep that might do it.