Skip to content
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

feat(api): add name argument to topk/value_counts #10083

Merged
merged 3 commits into from
Sep 11, 2024

Conversation

jcrist
Copy link
Member

@jcrist jcrist commented Sep 10, 2024

This adds a new name argument to topk & value_counts. Fixes #9948.

I think adding a name arg to these helpers makes sense. For other builtin aggregate methods you can chain a .name("myname") call on the end, but in these cases the return type is a Table. Adding an option to specify the name of the new column is nice for ux.

I opted to go for a simple name: str | None version, rather than supporting format strings/callables like rename. If a user wants to programmatically generate the name then they can do so themselves outside of this method. Keep it simple.

One open question is whether this argument should be keyword only. I went with positional or keyword for now, no strong thoughts.

Also added a docstring example for topk while I was at it.

@jcrist jcrist force-pushed the topk-value-counts-fixups branch from 46a2965 to 1625559 Compare September 10, 2024 21:26
@jcrist jcrist requested review from cpcloud and gforsyth September 10, 2024 21:44
Copy link
Member

@cpcloud cpcloud left a comment

Choose a reason for hiding this comment

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

Slight preference for keyword only, since both by and name can be strings, so without naming anything things can get slightly hard to read.

@jcrist jcrist force-pushed the topk-value-counts-fixups branch from 1625559 to 0c72c91 Compare September 11, 2024 01:51
@jcrist jcrist enabled auto-merge (rebase) September 11, 2024 02:02
@jcrist jcrist merged commit 24be184 into ibis-project:main Sep 11, 2024
81 checks passed
@jcrist jcrist deleted the topk-value-counts-fixups branch September 11, 2024 15:07
@NickCrews
Copy link
Contributor

This looks perfect, thanks @jcrist !

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

feat: add name param to .value_counts() and topk()
3 participants