From 3cfd4042f1b444141cd854bcd240746a60c0a97b Mon Sep 17 00:00:00 2001 From: shum461 Date: Thu, 15 Aug 2024 13:52:31 -0700 Subject: [PATCH 1/3] updated rlang messages in the predict functions to use cli. Fixes #1138 --- R/predict.R | 77 ++++++++++++++++++++++------------------------------- 1 file changed, 32 insertions(+), 45 deletions(-) diff --git a/R/predict.R b/R/predict.R index 327bd80ef..9219a56c4 100644 --- a/R/predict.R +++ b/R/predict.R @@ -147,7 +147,7 @@ #' @export predict.model_fit <- function(object, new_data, type = NULL, opts = list(), ...) { if (inherits(object$fit, "try-error")) { - rlang::warn("Model fit failed; cannot make predictions.") + cli::cli_warn("Model fit failed; cannot make predictions.") return(NULL) } @@ -156,7 +156,7 @@ predict.model_fit <- function(object, new_data, type = NULL, opts = list(), ...) type <- check_pred_type(object, type) if (type != "raw" && length(opts) > 0) { - rlang::warn("`opts` is only used with `type = 'raw'` and was ignored.") + cli::cli_warn("{.arg opts} is only used with type = 'raw' and was ignored.") } check_pred_type_dots(object, type, ...) @@ -173,7 +173,7 @@ predict.model_fit <- function(object, new_data, type = NULL, opts = list(), ...) linear_pred = predict_linear_pred(object = object, new_data = new_data, ...), hazard = predict_hazard(object = object, new_data = new_data, ...), raw = predict_raw(object = object, new_data = new_data, opts = opts, ...), - rlang::abort(glue::glue("I don't know about type = '{type}'")) + cli::cli_abort("I don't know about type = {.arg {type}}") ) if (!inherits(res, "tbl_spark")) { res <- switch( @@ -198,38 +198,34 @@ check_pred_type <- function(object, type, ...) { regression = "numeric", classification = "class", "censored regression" = "time", - rlang::abort("`type` should be 'regression', 'censored regression', or 'classification'.")) + cli::cli_abort("{.arg type} should be 'regression', 'censored regression', or 'classification'.")) } if (!(type %in% pred_types)) - rlang::abort( - glue::glue( - "`type` should be one of: ", - glue_collapse(pred_types, sep = ", ", last = " and ") - ) - ) + cli::cli_abort( + "{.arg type} should be one of:{.arg {pred_types}}") switch( type, "numeric" = if (object$spec$mode != "regression") { - rlang::abort("For numeric predictions, the object should be a regression model.") + cli::cli_abort("For numeric predictions, the object should be a regression model.") }, "class" = if (object$spec$mode != "classification") { - rlang::abort("For class predictions, the object should be a classification model.") + cli::cli_abort("For class predictions, the object should be a classification model.") }, "prob" = if (object$spec$mode != "classification") { - rlang::abort("For probability predictions, the object should be a classification model.") + cli::cli_abort("For probability predictions, the object should be a classification model.") }, "time" = if (object$spec$mode != "censored regression") { - rlang::abort("For event time predictions, the object should be a censored regression.") + cli::cli_abort("For event time predictions, the object should be a censored regression.") }, "survival" = if (object$spec$mode != "censored regression") { - rlang::abort("For survival probability predictions, the object should be a censored regression.") + cli::cli_abort("For survival probability predictions, the object should be a censored regression.") }, "hazard" = if (object$spec$mode != "censored regression") { - rlang::abort("For hazard predictions, the object should be a censored regression.") + cli::cli_abort("For hazard predictions, the object should be a censored regression.") }, "linear_pred" = if (object$spec$mode != "censored regression") { - rlang::abort("For the linear predictor, the object should be a censored regression.") + cli::cli_abort("For the linear predictor, the object should be a censored regression.") } ) @@ -349,56 +345,47 @@ check_pred_type_dots <- function(object, type, ..., call = rlang::caller_env()) other_args <- c("interval", "level", "std_error", "quantile", "time", "eval_time", "increasing") + + eval_time_types <- c("survival", "hazard") + is_pred_arg <- names(the_dots) %in% other_args if (any(!is_pred_arg)) { bad_args <- names(the_dots)[!is_pred_arg] bad_args <- paste0("`", bad_args, "`", collapse = ", ") - rlang::abort( - glue::glue( - "The ellipses are not used to pass args to the model function's ", - "predict function. These arguments cannot be used: {bad_args}", - ) + cli::cli_abort( + "The ellipses are not used to pass args to the model function's + predict function. These arguments cannot be used: {.val bad_args}", ) } # ---------------------------------------------------------------------------- # places where eval_time should not be given if (any(nms == "eval_time") & !type %in% c("survival", "hazard")) { - rlang::abort( - paste( - "`eval_time` should only be passed to `predict()` when `type` is one of:", - paste0("'", c("survival", "hazard"), "'", collapse = ", ") - ) - ) + cli::cli_abort("{.arg eval_time} should only be passed to {.fn predict} when {.arg type} is one of: + {.val {eval_time_types}}") + + } if (any(nms == "time") & !type %in% c("survival", "hazard")) { - rlang::abort( - paste( - "'time' should only be passed to `predict()` when 'type' is one of:", - paste0("'", c("survival", "hazard"), "'", collapse = ", ") - ) - ) + cli::cli_abort("{.arg time} should only be passed to {.fn predict} when {.arg type} is one of: + {.val {eval_time_types}}") } # when eval_time should be passed if (!any(nms %in% c("eval_time", "time")) & type %in% c("survival", "hazard")) { - rlang::abort( - paste( - "When using `type` values of 'survival' or 'hazard',", - "a numeric vector `eval_time` should also be given." - ) - ) + cli::cli_abort( + "When using `type` values of 'survival' or 'hazard' a numeric vector `eval_time` should also be given.") + } # `increasing` only applies to linear_pred for censored regression if (any(nms == "increasing") & !(type == "linear_pred" & object$spec$mode == "censored regression")) { - rlang::abort( - paste( - "The 'increasing' argument only applies to predictions of", - "type 'linear_pred' for the mode censored regression." + cli::cli_abort( + "The {.arg increasing} argument only applies to predictions of + type 'linear_pred' for the mode censored regression." ) - ) + } invisible(TRUE) From bd633c15e2bd5fc63f18f650bff38a41ddbea9df Mon Sep 17 00:00:00 2001 From: "Simon P. Couch" Date: Wed, 28 Aug 2024 08:56:26 -0500 Subject: [PATCH 2/3] apply suggestions from code review --- R/predict.R | 25 ++++++++++++++++--------- 1 file changed, 16 insertions(+), 9 deletions(-) diff --git a/R/predict.R b/R/predict.R index 9219a56c4..56d4f538b 100644 --- a/R/predict.R +++ b/R/predict.R @@ -156,7 +156,7 @@ predict.model_fit <- function(object, new_data, type = NULL, opts = list(), ...) type <- check_pred_type(object, type) if (type != "raw" && length(opts) > 0) { - cli::cli_warn("{.arg opts} is only used with type = 'raw' and was ignored.") + cli::cli_warn("{.arg opts} is only used with `type = 'raw'` and was ignored.") } check_pred_type_dots(object, type, ...) @@ -173,7 +173,7 @@ predict.model_fit <- function(object, new_data, type = NULL, opts = list(), ...) linear_pred = predict_linear_pred(object = object, new_data = new_data, ...), hazard = predict_hazard(object = object, new_data = new_data, ...), raw = predict_raw(object = object, new_data = new_data, opts = opts, ...), - cli::cli_abort("I don't know about type = {.arg {type}}") + cli::cli_abort("Unknown prediction {.arg type} '{type}'.") ) if (!inherits(res, "tbl_spark")) { res <- switch( @@ -355,14 +355,18 @@ check_pred_type_dots <- function(object, type, ..., call = rlang::caller_env()) cli::cli_abort( "The ellipses are not used to pass args to the model function's predict function. These arguments cannot be used: {.val bad_args}", + call = call ) } # ---------------------------------------------------------------------------- # places where eval_time should not be given if (any(nms == "eval_time") & !type %in% c("survival", "hazard")) { - cli::cli_abort("{.arg eval_time} should only be passed to {.fn predict} when {.arg type} is one of: - {.val {eval_time_types}}") + cli::cli_abort( + "{.arg eval_time} should only be passed to {.fn predict} when \\ + {.arg type} is one of {.or {.val {eval_time_types}}}", + call = call + ) } @@ -372,9 +376,11 @@ check_pred_type_dots <- function(object, type, ..., call = rlang::caller_env()) } # when eval_time should be passed if (!any(nms %in% c("eval_time", "time")) & type %in% c("survival", "hazard")) { - cli::cli_abort( - "When using `type` values of 'survival' or 'hazard' a numeric vector `eval_time` should also be given.") - + cli::cli_abort( + "When using {.arg type} values of {.or {.val {eval_time_types}}} a numeric + vector {.arg eval_time} should also be given.", + call = call + ) } # `increasing` only applies to linear_pred for censored regression @@ -382,8 +388,9 @@ check_pred_type_dots <- function(object, type, ..., call = rlang::caller_env()) !(type == "linear_pred" & object$spec$mode == "censored regression")) { cli::cli_abort( - "The {.arg increasing} argument only applies to predictions of - type 'linear_pred' for the mode censored regression." + "{.arg increasing} only applies to predictions of + type 'linear_pred' for the mode censored regression.", + call = call ) } From e0e0dc81f08f97f10422999320f2c48fd6c28910 Mon Sep 17 00:00:00 2001 From: simonpcouch Date: Wed, 28 Aug 2024 09:14:30 -0500 Subject: [PATCH 3/3] pass calls, use inline markup --- R/predict.R | 75 +++++++++++++++++++++++++++++++++++++---------------- 1 file changed, 53 insertions(+), 22 deletions(-) diff --git a/R/predict.R b/R/predict.R index 56d4f538b..e2a7545f2 100644 --- a/R/predict.R +++ b/R/predict.R @@ -191,41 +191,69 @@ predict.model_fit <- function(object, new_data, type = NULL, opts = list(), ...) res } -check_pred_type <- function(object, type, ...) { +check_pred_type <- function(object, type, ..., call = rlang::caller_env()) { if (is.null(type)) { type <- - switch(object$spec$mode, - regression = "numeric", - classification = "class", - "censored regression" = "time", - cli::cli_abort("{.arg type} should be 'regression', 'censored regression', or 'classification'.")) + switch( + object$spec$mode, + regression = "numeric", + classification = "class", + "censored regression" = "time", + cli::cli_abort( + "{.arg type} should be 'regression', 'censored regression', or 'classification'.", + call = call + ) + ) } if (!(type %in% pred_types)) cli::cli_abort( - "{.arg type} should be one of:{.arg {pred_types}}") + "{.arg type} should be one of:{.arg {pred_types}}", + call = call + ) switch( type, "numeric" = if (object$spec$mode != "regression") { - cli::cli_abort("For numeric predictions, the object should be a regression model.") + cli::cli_abort( + "For numeric predictions, the object should be a regression model.", + call = call + ) }, "class" = if (object$spec$mode != "classification") { - cli::cli_abort("For class predictions, the object should be a classification model.") + cli::cli_abort( + "For class predictions, the object should be a classification model.", + call = call + ) }, "prob" = if (object$spec$mode != "classification") { - cli::cli_abort("For probability predictions, the object should be a classification model.") + cli::cli_abort( + "For probability predictions, the object should be a classification model.", + call = call + ) }, "time" = if (object$spec$mode != "censored regression") { - cli::cli_abort("For event time predictions, the object should be a censored regression.") + cli::cli_abort( + "For event time predictions, the object should be a censored regression.", + call = call + ) }, "survival" = if (object$spec$mode != "censored regression") { - cli::cli_abort("For survival probability predictions, the object should be a censored regression.") + cli::cli_abort( + "For survival probability predictions, the object should be a censored regression.", + call = call + ) }, "hazard" = if (object$spec$mode != "censored regression") { - cli::cli_abort("For hazard predictions, the object should be a censored regression.") + cli::cli_abort( + "For hazard predictions, the object should be a censored regression.", + call = call + ) }, "linear_pred" = if (object$spec$mode != "censored regression") { - cli::cli_abort("For the linear predictor, the object should be a censored regression.") + cli::cli_abort( + "For the linear predictor, the object should be a censored regression.", + call = call + ) } ) @@ -363,16 +391,19 @@ check_pred_type_dots <- function(object, type, ..., call = rlang::caller_env()) # places where eval_time should not be given if (any(nms == "eval_time") & !type %in% c("survival", "hazard")) { cli::cli_abort( - "{.arg eval_time} should only be passed to {.fn predict} when \\ - {.arg type} is one of {.or {.val {eval_time_types}}}", + "{.arg eval_time} should only be passed to {.fn predict} when \\ + {.arg type} is one of {.or {.val {eval_time_types}}}.", call = call ) } if (any(nms == "time") & !type %in% c("survival", "hazard")) { - cli::cli_abort("{.arg time} should only be passed to {.fn predict} when {.arg type} is one of: - {.val {eval_time_types}}") + cli::cli_abort( + "{.arg time} should only be passed to {.fn predict} when {.arg type} is + one of {.or {.val {eval_time_types}}}.", + call = call + ) } # when eval_time should be passed if (!any(nms %in% c("eval_time", "time")) & type %in% c("survival", "hazard")) { @@ -388,10 +419,10 @@ check_pred_type_dots <- function(object, type, ..., call = rlang::caller_env()) !(type == "linear_pred" & object$spec$mode == "censored regression")) { cli::cli_abort( - "{.arg increasing} only applies to predictions of - type 'linear_pred' for the mode censored regression.", - call = call - ) + "{.arg increasing} only applies to predictions of + type 'linear_pred' for the mode censored regression.", + call = call + ) }