GH-15894: Make sure the functions that are supposed to be exported are exported in the R package #15899
Conversation
| @@ -48,6 +48,10 @@ | |||
| registerS3method("print", "H2OExplanation", "print.H2OExplanation") | |||
| .s3_register("repr", "repr_text", "H2OExplanation") | |||
| .s3_register("repr", "repr_html", "H2OExplanation") | |||
|
|
|||
| # CoxPH | |||
| .s3_register("survival", "survfit", "H2OCoxPHModel") | |||
There was a problem hiding this comment.
Registering a class with S3 method in one package to a generic from another package is hard in R (unless it's some of the packages that are loaded by default in the package environment (base) - for those we can use the registerS3Method).
|
|
||
| # src_path <- paste(h2oRDir,"h2o-package","R",sep=.Platform$file.sep) | ||
| # invisible(lapply(to_src,function(x){source(paste(src_path, x, sep = .Platform$file.sep))})) | ||
| devtools::load_all(paste(h2oRDir,"h2o-package",sep=.Platform$file.sep), export_all=FALSE) |
There was a problem hiding this comment.
export_all = FALSE tells devtools to just export those methods that are supposed to be exported so this helps to find out which methods are not exported but used in the tests.
| - h2o.get_predictor_added_per_step | ||
| - h2o.get_predictor_removed_per_step | ||
| - h2o.get_predictors_added_per_step | ||
| - h2o.get_predictors_removed_per_step |
There was a problem hiding this comment.
(missing s in predictorS )
tomasfryda
force-pushed
the
tomf_GH-15894_export_unexported_in_R
branch
from
ab693b1
to
6cfe711
Compare
| @@ -113,10 +113,13 @@ function(cur.dir, root, root.parent = NULL) { | |||
| default.packages <- | |||
| function() { | |||
| to_require <- c("jsonlite", "RCurl", "RUnit", "R.utils", "testthat", "ade4", "glmnet", "gbm", "ROCR", "e1071", | |||
| "tools", "statmod", "fpc", "cluster") | |||
| "tools", "statmod", "fpc", "cluster", "survival") | |||
| invisible(lapply(to_require,function(x){require(x,character.only=TRUE,quietly=TRUE,warn.conflicts=FALSE)})) | ||
| invisible(lapply(to_require,function(x) { | ||
| if (!require(x,character.only=TRUE,quietly=TRUE,warn.conflicts=FALSE)) | ||
| warning(paste("Package", x, "is not available."), call. = FALSE) |
There was a problem hiding this comment.
Mainly to make it easier for us to find out why some test might not work. We import packages and if unsuccessful we used to silently fail, after this change we should see that some package is not available (and R names are sometimes quite obscure so it's hard to guess if we are missing a package or badly exporting something or both (survfit.H2OCoxPHModel)).
|
I would like to ask you for a review in your areas:
|
| .s3_register("survival", "survfit", "H2OCoxPHModel") | ||
|
|
||
| # H2OFrame | ||
| .s3_register("stats", "median", "H2OFrame") |
There was a problem hiding this comment.
stats package is not imported in the package namespace so it has to be specified here.
Confusingly, min, mean, max are defined in base package that is imported so they don't have to be listed here.
#15894