njtierney / naniar

Tidy data structures, summaries, and visualisations for missing data
http://naniar.njtierney.com/
Other
651 stars 53 forks source link

Updated coerce_fct_na_explicit #250

Closed stulacy closed 4 years ago

stulacy commented 4 years ago

This function was previously returning NULL when the factor had no missing values. I've modified it to return the input if it has no missing values.

Description

The gg_miss_fct function wasn't working for a dataset I was using. I investigated and found the case being that the code attempts to reencode all NAs in the fct argument using the in-built function coerce_fct_na_explicit. However, coerce_fct_no_explicit has no default return value, so if your factor fails the If check, the function returns NULL and thus gg_miss_fct fails later on.

I've resolved this by simply adding an Else statement to provide a default return value, being the input factor.

Example

Example below that fails using the current Master branch.

devtools::install_github("njtierney/naniar")
library(naniar)

# Works as expected
gg_miss_fct(x = riskfactors, fct = marital)

# have 245 observations of 34 values
dim(riskfactors)
# Have 1 missing value in marital column
sum(is.na(riskfactors$marital))
# Removing this row gives us 244 x 34
riskfactors_nomiss <- riskfactors[!is.na(riskfactors$marital), ]
dim(riskfactors_nomiss)
# And now the function doesn't work
gg_miss_fct(x = riskfactors_nomiss, fct = marital)

Running this example under my fork works:

# Install my fork
devtools::install_github("stulacy/naniar")
library(naniar)

# Default method still works
gg_miss_fct(x = riskfactors, fct = marital)

# And now the function works correctly when no missing in marital column
riskfactors_nomiss <- riskfactors[!is.na(riskfactors$marital), ]
gg_miss_fct(x = riskfactors_nomiss, fct = marital)

Tests

I haven't run any automated testing. Let me know if this will be needed.

NEWS + DESCRIPTION

This is a bug fix so I don't believe the DESCRIPTION or NEWS.md need updating.

njtierney commented 4 years ago

Wow, thank you so much! That is just fantastic.

I myself ran into this problem yesterday - I've documented this at #251, I think that it should be resolved by this, but there looks like there is one case that it doesn't quite solve.

Merged in, thank you again!