Skip to content

Conversation

@arw2019
Copy link
Contributor

@arw2019 arw2019 commented Nov 10, 2020

  • closes #xxxx
  • tests added / passed
  • passes black pandas
  • passes git diff upstream/master -u -- "*.py" | flake8 --diff
  • whatsnew entry

@WillAyd
Copy link
Member

WillAyd commented Nov 10, 2020

Does this have any impact to an end user?

@arw2019
Copy link
Contributor Author

arw2019 commented Nov 10, 2020

Does this have any impact to an end user?

Good point. I think it allows the user to pass min_count to group_nth. Will see if I can come up with a test

if agg_func is libgroupby.group_nth:
# different signature from the others
# TODO: should we be using min_count instead of hard-coding it?
agg_func(result, counts, values, comp_ids, rank=1, min_count=-1)
Copy link
Member

Choose a reason for hiding this comment

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

one alternative here is to remove the keyword from the libgroupby function.

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 went with that

Actually we don't use the min_count argument in the libgroupby function - I guess that it was put there to homogenize signatures, but now they're different anyways

@arw2019 arw2019 changed the title CLN: don't hard-code min_count in call to _libs.groupby.group_nth CLN: remove unused min_count argument in _libs.groupby.group_nth Nov 11, 2020
@arw2019 arw2019 changed the title CLN: remove unused min_count argument in _libs.groupby.group_nth CLN: remove unused min_count argument in libgroupby.group_nth Nov 11, 2020
@jreback jreback added the Clean label Nov 13, 2020
@jreback jreback added this to the 1.2 milestone Nov 13, 2020
@jreback jreback merged commit c4f6588 into pandas-dev:master Nov 13, 2020
@jreback
Copy link
Contributor

jreback commented Nov 13, 2020

thanks @arw2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants