Conversation
FutureWarning to propagate to the user, but it doesn't require any additional action on the Dask side.
jrbourbeau
left a comment
There was a problem hiding this comment.
Thanks for handling all of this @ian-r-rose
dask/dataframe/groupby.py
Outdated
|
|
||
| def _groupby_slice_apply( | ||
| df, grouper, key, func, *args, group_keys=True, dropna=None, observed=None, **kwargs | ||
| df, grouper, key, func, *args, group_keys=None, dropna=None, observed=None, **kwargs |
There was a problem hiding this comment.
Should these also be using GROUP_KEYS_DEFAULT?
There was a problem hiding this comment.
Sure. I'm not sure it matters since this is only invoked from the top-level functions that already have the default set, but I don't see the harm.
|
|
||
|
|
||
| def test_groupby_group_keys(): | ||
| @pytest.mark.parametrize("group_keys", [True, False, None]) |
There was a problem hiding this comment.
Nice -- thank you for expanding this test
|
Thanks for the heads up @ian-r-rose - Sorry I missed this. It looks like cudf does not support anything other than |
|
There's a nice explanation of the change here https://pandas.pydata.org/docs/dev/whatsnew/v1.5.0.html#using-group-keys-with-transformers-in-groupby-apply |
|
Unfortunately, the changelog entry is not fully consistent with the actual code, which changed the defaults to |
|
@shwina @galipremsagar - Is this |
|
Thanks for taking this on @ian-r-rose! If we want to pass cudf tests we could xfail or skip the ones where |
There was a problem hiding this comment.
Thanks @ian-r-rose! This should be good to go after CI finishes
EDIT: Just merged main to include #8986 which should fix some unrelated CI failures
…test-upstream]
|
Woo! |
pre-commit run --all-filesI've filtered a number of warnings with links to some new relevant issues as this has been dragging on for a while, and we should really get our upstream CI green.