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

Factor to indicators error message #396

Open
wants to merge 6 commits into
base: devel
Choose a base branch
from

Conversation

tq21
Copy link
Contributor

@tq21 tq21 commented Sep 19, 2022

Fix issue #285. In addition, I added a check to ensure all covariates and outcomes have more than one category (categorical variable) or unique value (continuous variable). Code will fail immediately if users try to define an sl3_Task with data that contains columns with one value. An error message indicating the names of those columns will be printed.

Sky Qiu and others added 4 commits August 30, 2022 16:04
throw a warning for constant outcome(s) and covariates, and drop constant constant covariates
@rachaelvp
Copy link
Member

rachaelvp commented Sep 19, 2022

For covariates and outcome(s) that are constant (i.e., have one unique value/level), we only need to throw a warning. The error here is not necessary, e.g. imagine if someone creates a task for prediction with just one observation

@rachaelvp
Copy link
Member

Hi @nhejazi and/or @jeremyrcoyle could you please take a look at the test errors here? I am not sure what's going on. Perhaps an error with GitHub Actions R with Java, or maybe we need to change the Java parameters for Lrnr_bartMachine? I don't think the changes in this PR are causing the error.

@nhejazi
Copy link
Member

nhejazi commented Nov 11, 2022

@rachaelvp and @tq21 sorry I missed this (not sure how), but the failure seems only be related to a weird issue with installing devtools (maybe the package was somehow briefly unavailable on CRAN, as the same issue affects builds for #398). I'm not sure why devtools is even present under Suggests -- it seems to only appear in blocks like this (

if (FALSE) {
setwd("..")
setwd("..")
getwd()
library("devtools")
document()
load_all("./") # load all R files in /R and datasets in /data. Ignores NAMESPACE:
devtools::check() # runs full check
setwd("..")
install("sl3", build_vignettes = FALSE, dependencies = FALSE) # INSTALL W/ devtools:
}
) that precede some units tests and that really shouldn't be included at all

@nhejazi
Copy link
Member

nhejazi commented Nov 11, 2022

@rachaelvp if you've reviewed these changes and they look good, then my suggestion would be to merge into devel and then fix on the backend, as per my comment for #398; happy to help

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.

3 participants