-
Notifications
You must be signed in to change notification settings - Fork 1
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
Refactor eta_covariate to accept integer data #104
Conversation
The CI jobs for R 4.0 and 4.1 now fail due to a GGally installation failure triggered by the latest ggstats requiring R 4.2. Trying to resolve that with a targeted pin (like is currently done for evaluate), reveals more issues (broom.helpers requires at least R 4.2, emmeans and survey require at least R 4.1). Rather than chase these targeted pins, just pin RSPM to a known good state (date of the last successful CI run) for R versions before 4.2. And since we're using a date-pinned URL, drop the targeted pin for evaluate and choose a snapshot that has evaluate v0.23. Re: #104
@@ -10,7 +10,7 @@ diagnostic_display_list <- function(df, x, y, fun_cat, fun_cont) { | |||
out[[i]] <- lapply(seq_along(x), function(ii) { | |||
col <- col_label(x[[ii]])[[1]] | |||
require_column(df, col) | |||
if(inherits(unlist(df[, col]), c("character", "factor", "logical", "integer"))) { | |||
if(.is_discrete(df[[col]])) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Moved this decision into a helper function to unify across the package.
.is_continuous <- function(x) { | ||
inherits(x, c("integer", "numeric")) | ||
} | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't know why this code was so convoluted before.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
Making me pull out the dictionary. |
See #103
The primary finding was an error that is generated when you pass integer x data into
eta_covariate()
; that function dispatches tocont_cat()
which will generate an error, asking forfactor
,character
orlogical
. So that dispatch is fixed.This PR also re-factors how discrete and continuous data are detected in checks across the package. This ensures consistent adjudication in different places, specifically the handling when calling
eta_covariate()
.