Skip to content

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

Merged
martinctc merged 1 commit into
microsoft:mainfrom
DavisVaughan:fix/replace-na-only-on-numeric
Nov 20, 2021
Merged

Fix: only call replace_na(., 0) on numeric columns#195
martinctc merged 1 commit into
microsoft:mainfrom
DavisVaughan:fix/replace-na-only-on-numeric

Conversation

@DavisVaughan

Copy link
Copy Markdown
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

ghost commented Nov 18, 2021

Copy link
Copy Markdown

CLA assistant check
All CLA requirements met.

@martinctc

Copy link
Copy Markdown
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
Copy Markdown
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