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

Fix: only call replace_na(., 0) on numeric columns #195

Merged

Conversation

DavisVaughan
Copy link
Contributor

We are updating tidyr::replace_na() to utilize the vctrs package, and that results in slightly stricter / more correct type conversions. See tidyverse/tidyr#1219

We noticed in revdeps that this package broke. An easy way to see this is by installing the above mentioned PR and running:

library(wpa)

# Create a sample small dataset
orgs <- c("Customer Service", "Financial Planning", "Biz Dev")
em_data <- em_data[em_data$Organization %in% orgs, ]

# Return visualization of percentage distribution
workpatterns_area(em_data, return = "plot", values = "percent")
#> Error: Problem with `mutate()` column `PersonId`.
#> ℹ `PersonId = (structure(function (..., .x = ..1, .y = ..2, . = ..1) ...`.
#> x Can't convert `replace` <double> to match type of `data` <character>.

The problem boils down to the fact that you are calling mutate_all(df, ~replace_na(., 0)) on a df that has a mixture of character and numeric columns. Notably, PersonId and group are character columns, and you can no longer use a numeric 0 as a replacement value for these.

It seems like you probably just wanted to replace NAs in numeric columns, so this PR changes to a mutate() call that targets only numeric columns.

We would greatly appreciate if you could merge this PR and submit a patch release of your package to CRAN so we can send tidyr in!

@ghost
Copy link

ghost commented Nov 18, 2021

CLA assistant check
All CLA requirements met.

@martinctc
Copy link
Member

Thank you @DavisVaughan - will review this change soon as I can and push a new version to CRAN.

@martinctc martinctc changed the title Only call replace_na(., 0) on numeric columns Fix: only call replace_na(., 0) on numeric columns Nov 20, 2021
@martinctc martinctc merged commit 354acb9 into microsoft:main Nov 20, 2021
@martinctc
Copy link
Member

@DavisVaughan - can confirm that these changes have been merged and pushed to CRAN, in v1.6.3. Thanks again

@DavisVaughan DavisVaughan deleted the fix/replace-na-only-on-numeric branch January 11, 2022 14:36
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.

2 participants