tidyverse / forcats

🐈🐈🐈🐈: tools for working with categorical variables (factors)
https://forcats.tidyverse.org/
Other
554 stars 126 forks source link

fct_lump_n uses partial argument matching #276

Closed Zedseayou closed 3 years ago

Zedseayou commented 4 years ago

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. https://github.com/tidyverse/forcats/blob/ab81d1bf8ef8d1b776dd2a2030e9c664e54df239/R/lump.R#L149-L154

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 commented 4 years ago

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'