Skip to content

BUG: Fix agg with single-element function list given arg and kwargs #50725

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Conversation

luke396
Copy link
Contributor

@luke396 luke396 commented Jan 13, 2023

@luke396 luke396 changed the title BUG: Fix agg with one element func list given arg and kwargs BUG: Fix agg with single-element function list given arg and kwargs Jan 13, 2023
@mroeschke mroeschke requested a review from rhshadrach January 13, 2023 18:21
@mroeschke mroeschke added Groupby Apply Apply, Aggregate, Transform, Map labels Jan 13, 2023
@luke396 luke396 force-pushed the fix-agg-with-one-element-fuc-list-given-arg-kwagrs branch from 2afde21 to 7c4f6bc Compare January 14, 2023 04:05
Copy link
Member

@rhshadrach rhshadrach left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the PR! Unless it's a regression or some exceptional circumstances, we target the next minor version for the whatsnew. Right now, that's 2.0.0.

elif (
(args or kwargs)
and not isinstance(func, str)
and is_one_element_callable_list(func)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is going to fracture the behavior. Currently all list-like func are treated the same - it is the wrong behavior, but it's at least consistent. I think we need to find a better solution for passing through args and kwargs.

We save the args and kwargs to an instance on L112-113. Can we use them when we do the compute down below?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, we do need a better solution. I am going to try to solve this with their instance on L112-L113.

There is also the question of whether we should now focus on solving a single-elements function list, or should we consider the case of more elements.

For more elements, my initial approach was to use something like inspect.getargspec or inspect.signature. But if the parameters between several functions have the same naming or other conflicts, the problem seems to be more difficult.

I haven't fully sorted out my thoughts yet, so I've come up with a somewhat strange solution to a single-element list.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think we should only fix the single-element case as this would further fragment behavior for users. We should either fix all cases or leave behavior as-is.

For more elements, my initial approach was to use something like inspect.getargspec or inspect.signature. But if the parameters between several functions have the same naming or other conflicts, the problem seems to be more difficult.

I believe you're thinking of only passing arguments if the function accepts them. I don't think we should do that - consider in particular the case where a function accepts *args or **kwargs. Also, a user might have

def func(x, myarg=1):
    ...

and call ....agg(func, my_arg=2). This code should raise - the user has a typo. Instead it would silently produce incorrect results.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I see, I did not consider the typo problem, thanks for pointing it out!

I will try to solve the multi-element list directly, but it may take a little time.

@luke396 luke396 closed this Jan 18, 2023
@luke396 luke396 force-pushed the fix-agg-with-one-element-fuc-list-given-arg-kwagrs branch from 7c4f6bc to 3f0af5e Compare January 18, 2023 14:02
@luke396 luke396 deleted the fix-agg-with-one-element-fuc-list-given-arg-kwagrs branch January 18, 2023 14:07
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Apply Apply, Aggregate, Transform, Map Groupby
Projects
None yet
Development

Successfully merging this pull request may close these issues.

BUG: agg ignores args/kwargs when giving a list-like argument
3 participants