Partial functions in aggs may have arguments #9724

j-bennet · 2022-12-07T05:14:52Z

When the user passes a partial function into an aggregation, parameters should not be lost.

Closes Fully support partial functions as aggregations #9615
Tests added / passed
Passes pre-commit run --all-files

TODOs:

make sure documentation matches the behavior
handle positional args
what is known_np_funcs being used for?
anything else from kwargs besides ddof that std needs?
do other agg functions need args passed down?

This fixes the incorrect calculation, but may need a follow-up. In Pandas std and var accept another parameter that I'm not handling here: numeric_only. I'm not sure how to handle it here - would need more digging into what Pandas does with it.

GPUtester · 2022-12-07T05:14:54Z

Can one of the admins verify this patch?

Admins can comment ok to test to allow this one PR to run or add to allowlist to allow all future PRs from the same author to run.

ncclementi · 2022-12-07T14:53:36Z

add to allowlist

jrbourbeau

Thanks @j-bennet!

I see this is marked as a draft still -- are there additional changes you'd still like to make?

dask/dataframe/tests/test_groupby.py

jrbourbeau · 2022-12-07T17:42:44Z

dask/dataframe/groupby.py

@@ -1118,7 +1146,8 @@ def _finalize_mean(df, sum_column, count_column):
    return df[sum_column] / df[count_column]


-def _finalize_var(df, count_column, sum_column, sum2_column, ddof=1):
+def _finalize_var(df, count_column, sum_column, sum2_column, **kwargs):
+    ddof = kwargs.get("ddof", 1)


IIUC it looks like even though we're forwarding all kwargs specified by the user, we're only using ddof. Is that the case? If so, it'd be good to raise an error if a user passes in a kwarg that isn't supported. Rather raise an informative error than silently ignore the kwarg

Yes, this is one of my TODOs and a reason this PR is a draft. I have to see what else I need to pass down, and figure out how to handle the rest. I think I need to see what Pandas does if I pass unexpected parameters, and match the behavior.

I added handling for unexpected args, and a test for that. numeric_only kwarg is an interesting one - Pandas supports it, and I'm not sure why Dask doesn't, and whether it should.

numeric_only is a tricky one, as pandas behavior is changing. See #9471 (comment) . For now we have been filtering the warnings that pandas raises, see this PR #9496 .

@jrbourbeau I can't recall where we are in the discussion of this kwarg.

@ncclementi Thank you, I looked over the PRs, and it looks to me that numeric_only would have to be tackled separately, since it would be a pretty large change, and there's no clearly defined path yet.

j-bennet · 2022-12-07T18:57:38Z

I see this is marked as a draft still -- are there additional changes you'd still like to make?

Yes, I left several "note-to-self TODO" checkboxes on this PR, and I didn't have the chance to work on those yet.

Co-authored-by: James Bourbeau <jrbourbeau@users.noreply.github.com>

j-bennet · 2022-12-08T21:20:28Z

@jrbourbeau I believe this is ready for review.

ncclementi

@j-bennet This is looking good! I left some comments and suggestions for when you have time to look at it.

ncclementi · 2022-12-08T21:32:30Z

dask/dataframe/tests/test_groupby.py

+    )
+    ddf = dd.from_pandas(pdf, npartitions=2)
+
+    with pytest.raises((FutureWarning, TypeError)):


We can avoid testing pandas here, as this should be covered on their end.

This was actually intentional. I was thinking along the lines "if Pandas changes their behavior, we should catch it here and change accordingly". If you think this is an overkill, I'll remove it.

I see the idea.

In general, when there is a pandas change we want to be aware and we usually identify these ones when we run the upstream workflow. Then we can make the modifications catching the right message.

I would recommend removing testing pandas here.

dask/dataframe/tests/test_groupby.py

dask/dataframe/groupby.py

…back.

ncclementi · 2022-12-09T15:30:26Z

@j-bennet Thank you for your work, this LGTM .

@jrbourbeau do you have any more comments here ?

jrbourbeau

Thanks @j-bennet @ncclementi. I left a few final comments. They're all minor, overall this PR looks great. Looking forward to seeing it merged

dask/dataframe/groupby.py

dask/dataframe/tests/test_groupby.py

jrbourbeau · 2022-12-09T17:39:39Z

dask/dataframe/groupby.py

+    for arg in unexpected_kwargs:
+        raise TypeError(
+            f"aggregate function '{func}' got an unexpected keyword argument '{arg}'"
+        )


For multiple unexpected kwargs, it'd probably be a bit more informative to just let users see all the invalid kwargs at once. Otherwise, it may take some iteration to get rid of them all.

Suggested change

for arg in unexpected_kwargs:

raise TypeError(

f"aggregate function '{func}' got an unexpected keyword argument '{arg}'"

)

if unexpected_kwargs:

raise TypeError(

f"The aggregate function '{func}' supports {expected_kwargs} keyword arguments, but got {unexpected_kwargs}"

)

Note we'll need to make a corresponding change to test_groupby_aggregate_partial_function_unexpected_kwargs since we're updating the error message

dask/dataframe/groupby.py

Co-authored-by: James Bourbeau <jrbourbeau@users.noreply.github.com>

j-bennet · 2022-12-09T20:58:16Z

Fixed all the fine points from the review. Thanks for the suggestions!

jrbourbeau

Thanks @j-bennet! Will merge once CI finishes

jrbourbeau · 2022-12-09T21:55:56Z

Also, I noticed this is your first code contribution to this repository. Welcome!

mrocklin · 2022-12-09T22:37:48Z

Glad to see this in. Welcome @j-bennet !

j-bennet · 2022-12-10T00:04:40Z

@jrbourbeau

Also, I noticed this is your first code contribution to this repository. Welcome!

Second one! But the first one was a long time ago :). This: #3828

jrbourbeau · 2022-12-10T00:22:53Z

Ah, in that case -- welcome back! : )

Partial functions in aggs may have arguments.

5b340aa

github-actions bot added the dataframe label Dec 7, 2022

j-bennet marked this pull request as draft December 7, 2022 05:23

jrbourbeau reviewed Dec 7, 2022

View reviewed changes

j-bennet and others added 2 commits December 7, 2022 11:08

Test with multiple partitions.

08972d4

Co-authored-by: James Bourbeau <jrbourbeau@users.noreply.github.com>

Handle unexpected args in partials.

ac29a7c

j-bennet changed the title ~~[WIP] Partial functions in aggs may have arguments.~~ Partial functions in aggs may have arguments. Dec 8, 2022

j-bennet marked this pull request as ready for review December 8, 2022 01:28

j-bennet requested a review from jrbourbeau December 8, 2022 01:28

ncclementi reviewed Dec 8, 2022

View reviewed changes

Added positional args test case, updated kwargs case with review feed…

bb9f70b

…back.

j-bennet requested review from ncclementi and removed request for jrbourbeau December 9, 2022 05:55

ncclementi approved these changes Dec 9, 2022

View reviewed changes

jrbourbeau reviewed Dec 9, 2022

View reviewed changes

j-bennet and others added 4 commits December 9, 2022 12:01

Compute calls are not necessary, exceptions happens without computing

dc2d707

Co-authored-by: James Bourbeau <jrbourbeau@users.noreply.github.com>

Reword exceptons, and complain about all kwargs at the same time.

9eb52e7

Fix typo

ae95ec3

Co-authored-by: James Bourbeau <jrbourbeau@users.noreply.github.com>

No need for set

cd56f25

Co-authored-by: James Bourbeau <jrbourbeau@users.noreply.github.com>

jrbourbeau changed the title ~~Partial functions in aggs may have arguments.~~ Partial functions in aggs may have arguments Dec 9, 2022

jrbourbeau approved these changes Dec 9, 2022

View reviewed changes

jrbourbeau merged commit 159948e into dask:main Dec 9, 2022

j-bennet deleted the j-bennet/9615-fix-partial-aggs branch December 10, 2022 00:05

j-bennet mentioned this pull request Dec 12, 2022

Fully support partial functions as aggregations #9615

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Partial functions in aggs may have arguments #9724

Partial functions in aggs may have arguments #9724

j-bennet commented Dec 7, 2022 •

edited

GPUtester commented Dec 7, 2022

ncclementi commented Dec 7, 2022

jrbourbeau left a comment •

edited

jrbourbeau Dec 7, 2022

j-bennet Dec 7, 2022

j-bennet Dec 8, 2022 •

edited

ncclementi Dec 8, 2022

j-bennet Dec 8, 2022

j-bennet commented Dec 7, 2022

j-bennet commented Dec 8, 2022

ncclementi left a comment

ncclementi Dec 8, 2022

j-bennet Dec 9, 2022

ncclementi Dec 9, 2022

ncclementi commented Dec 9, 2022

jrbourbeau left a comment

jrbourbeau Dec 9, 2022

jrbourbeau Dec 9, 2022

j-bennet commented Dec 9, 2022

jrbourbeau left a comment

jrbourbeau commented Dec 9, 2022

mrocklin commented Dec 9, 2022

j-bennet commented Dec 10, 2022

jrbourbeau commented Dec 10, 2022

Partial functions in aggs may have arguments #9724

Partial functions in aggs may have arguments #9724

Conversation

j-bennet commented Dec 7, 2022 • edited

GPUtester commented Dec 7, 2022

ncclementi commented Dec 7, 2022

jrbourbeau left a comment • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

j-bennet Dec 8, 2022 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

j-bennet commented Dec 7, 2022

j-bennet commented Dec 8, 2022

ncclementi left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ncclementi commented Dec 9, 2022

jrbourbeau left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

j-bennet commented Dec 9, 2022

jrbourbeau left a comment

Choose a reason for hiding this comment

jrbourbeau commented Dec 9, 2022

mrocklin commented Dec 9, 2022

j-bennet commented Dec 10, 2022

jrbourbeau commented Dec 10, 2022

j-bennet commented Dec 7, 2022 •

edited

jrbourbeau left a comment •

edited

j-bennet Dec 8, 2022 •

edited