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

fct_lump_n uses partial argument matching #276

Closed
Zedseayou opened this issue Oct 12, 2020 · 1 comment · Fixed by #268
Closed

fct_lump_n uses partial argument matching #276

Zedseayou opened this issue Oct 12, 2020 · 1 comment · Fixed by #268

Comments

@Zedseayou
Copy link

Zedseayou commented Oct 12, 2020

fct_lump_n uses partial argument matching when it calls rank, which generates a lot of warnings if you have options(warnPartialMatchArgs = TRUE) and repeatedly call the function (e.g. in a grouped operation).

library(forcats)
options(warnPartialMatchArgs = TRUE)
x <- c("a", "a", "a", "b", "b", "c", "d")
fct_lump_n(x, n = 2)
#> Warning in rank(-calcs$count, ties = ties.method): partial argument match of
#> 'ties' to 'ties.method'
#> [1] a     a     a     b     b     Other Other
#> Levels: a b Other

I think the culprit is here, with the named argument given as ties instead of ties.method.

forcats/R/lump.R

Lines 149 to 154 in ab81d1b

if (n < 0) {
rank <- rank(calcs$count, ties = ties.method)
n <- -n
} else {
rank <- rank(-calcs$count, ties = ties.method)
}

I haven't tried making the change but happy to PR if this would be welcome. I realise rank is unlikely to change signature but at least for me squelching the warnings would be great

@jennybc
Copy link
Member

jennybc commented Oct 12, 2020

Addressed by existing PR #268

Why doesn't CI surface this? 🤔 If I have similar code in a package, locally I see:

N  checking R code for possible problems (6.4s)
   foofy: warning in rank(1:4, ties = "random"): partial argument match of
     'ties' to 'ties.method'

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 a pull request may close this issue.

2 participants