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

Add weight argument to fct_infreq #330

Merged
merged 5 commits into from Jan 4, 2023
Merged

Add weight argument to fct_infreq #330

merged 5 commits into from Jan 4, 2023

Conversation

hadley
Copy link
Member

@hadley hadley commented Jan 3, 2023

Fixes #261

R/reorder.R Outdated
@@ -134,10 +134,23 @@ fct_inorder <- function(f, ordered = NA) {

#' @export
#' @rdname fct_inorder
fct_infreq <- function(f, ordered = NA) {
#' @param weight Optional vector of numeric values to weight by.
fct_infreq <- function(f, weight = NULL, ordered = NA) {
Copy link
Member Author

Choose a reason for hiding this comment

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

Is weight the right name?

Copy link
Member

Choose a reason for hiding this comment

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

fct_lump() and the other functions in that family use w FWIW

I don't think there are any other weighted forcats functions to compare with.

NEWS.md Outdated Show resolved Hide resolved
R/reorder.R Outdated
@@ -134,10 +134,23 @@ fct_inorder <- function(f, ordered = NA) {

#' @export
#' @rdname fct_inorder
fct_infreq <- function(f, ordered = NA) {
#' @param weight Optional vector of numeric values to weight by.
fct_infreq <- function(f, weight = NULL, ordered = NA) {
Copy link
Member

Choose a reason for hiding this comment

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

fct_lump() and the other functions in that family use w FWIW

I don't think there are any other weighted forcats functions to compare with.

R/reorder.R Outdated Show resolved Hide resolved
R/reorder.R Outdated Show resolved Hide resolved
tests/testthat/test-reorder.R Outdated Show resolved Hide resolved
R/reorder.R Outdated Show resolved Hide resolved
R/reorder.R Outdated
Comment on lines 137 to 138
fct_infreq <- function(f, ordered = NA) {
#' @param weight Optional vector of numeric values to weight by.
fct_infreq <- function(f, weight = NULL, ordered = NA) {
Copy link
Member

Choose a reason for hiding this comment

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

Do you expect the argument ordering here to matter? Probably not much? i.e. if someone specified ordered without naming the argument. Seems unlikely

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah, I think that seems ok

hadley and others added 4 commits January 4, 2023 12:03
Co-authored-by: Davis Vaughan <davis@rstudio.com>
#Conflicts:
#	NEWS.md
#	tests/testthat/test-reorder.R
@hadley hadley merged commit 6f01b05 into main Jan 4, 2023
@hadley hadley deleted the weighted-infreq branch January 4, 2023 19:01
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.

fct_infreq() should support weights
2 participants