create helper for newdata error (#912)

simonpcouch · web-flow · commit 00f3cdb5500b · 2023-03-10T12:41:30.000-05:00
diff --git a/NEWS.md b/NEWS.md
@@ -15,8 +15,12 @@
 * Fixed bug with prediction from a boosted tree model fitted with `"xgboost"` using a custom objective function (#875).
 
 * Several internal functions (to help work with `Surv` objects) were added as a standalone file that can be used in other packages via `usethis::use_standalone("tidymodels/parsnip")`. 
+
+* Rather than being implemented in each method, the check for the `new_data` argument being mistakenly passed as `newdata` to `multi_predict()` now happens in the generic. Packages re-exporting the `multi_predict()` generic and implementing now-duplicate checks may see new failures and can remove their own analogous checks. This check already existed in all `predict()` methods (via `predict.model_fit()`) and all parsnip `multi_predict()` methods (#525).
+
 * `logistic_reg()` will now warn at `fit()` when the outcome has more than two levels (#545).
 
+
 # parsnip 1.0.4
 
 * For censored regression models, a "reverse Kaplan-Meier" curve is computed for the censoring distribution. This can be used when evaluating this type of model (#855).
diff --git a/R/aaa_multi_predict.R b/R/aaa_multi_predict.R
@@ -22,6 +22,7 @@ multi_predict <- function(object, ...) {
     rlang::warn("Model fit failed; cannot make predictions.")
     return(NULL)
   }
+  check_for_newdata(...)
   UseMethod("multi_predict")
 }
 
diff --git a/R/boost_tree.R b/R/boost_tree.R
@@ -480,10 +480,6 @@ get_event_level <- function(model_spec){
 #' @param trees An integer vector for the number of trees in the ensemble.
 multi_predict._xgb.Booster <-
   function(object, new_data, type = NULL, trees = NULL, ...) {
-    if (any(names(enquos(...)) == "newdata")) {
-      rlang::abort("Did you mean to use `new_data` instead of `newdata`?")
-    }
-
     if (is.null(trees)) {
       trees <- object$fit$nIter
     }
@@ -608,9 +604,6 @@ C5.0_train <-
 #' @rdname multi_predict
 multi_predict._C5.0 <-
   function(object, new_data, type = NULL, trees = NULL, ...) {
-    if (any(names(enquos(...)) == "newdata"))
-      rlang::abort("Did you mean to use `new_data` instead of `newdata`?")
-
     if (is.null(trees))
       trees <- min(object$fit$trials)
     trees <- sort(trees)
diff --git a/R/glmnet-engines.R b/R/glmnet-engines.R
@@ -173,11 +173,6 @@ multi_predict_glmnet <- function(object,
                                  type = NULL,
                                  penalty = NULL,
                                  ...) {
-
-  if (any(names(enquos(...)) == "newdata")) {
-    rlang::abort("Did you mean to use `new_data` instead of `newdata`?")
-  }
-
   if (object$spec$mode == "classification") {
     if (is_quosure(penalty)) {
       penalty <- eval_tidy(penalty)
diff --git a/R/mars.R b/R/mars.R
@@ -150,9 +150,6 @@ earth_reg_updater <- function(num, object, new_data, ...) {
 #' @export
 multi_predict._earth <-
   function(object, new_data, type = NULL, num_terms = NULL, ...) {
-    if (any(names(enquos(...)) == "newdata"))
-      rlang::abort("Did you mean to use `new_data` instead of `newdata`?")
-
     load_libs(object, quiet = TRUE, attach = TRUE)
 
     if (is.null(num_terms))
diff --git a/R/misc.R b/R/misc.R
@@ -483,3 +483,13 @@ check_case_weights <- function(x, spec) {
   }
   invisible(NULL)
 }
+
+# -----------------------------------------------------------------------------
+check_for_newdata <- function(..., call = rlang::caller_env()) {
+  if (any(names(list(...)) == "newdata")) {
+    rlang::abort(
+      "Please use `new_data` instead of `newdata`.",
+      call = call
+    )
+  }
+}
diff --git a/R/mlp.R b/R/mlp.R
@@ -366,9 +366,6 @@ mlp_num_weights <- function(p, hidden_units, classes) {
 #' @export
 multi_predict._torch_mlp <-
   function(object, new_data, type = NULL, epochs = NULL, ...) {
-    if (any(names(enquos(...)) == "newdata"))
-      rlang::abort("Did you mean to use `new_data` instead of `newdata`?")
-
     load_libs(object, quiet = TRUE, attach = TRUE)
 
     if (is.null(epochs))
diff --git a/R/nearest_neighbor.R b/R/nearest_neighbor.R
@@ -149,9 +149,6 @@ translate.nearest_neighbor <- function(x, engine = x$engine, ...) {
 #' @export
 multi_predict._train.kknn <-
   function(object, new_data, type = NULL, neighbors = NULL, ...) {
-    if (any(names(enquos(...)) == "newdata"))
-      rlang::abort("Did you mean to use `new_data` instead of `newdata`?")
-
     if (is.null(neighbors))
       neighbors <- rlang::eval_tidy(object$fit$call$ks)
     neighbors <- sort(neighbors)
diff --git a/R/predict.R b/R/predict.R
@@ -324,15 +324,13 @@ make_pred_call <- function(x) {
   cl
 }
 
-check_pred_type_dots <- function(object, type, ...) {
+check_pred_type_dots <- function(object, type, ..., call = rlang::caller_env()) {
   the_dots <- list(...)
   nms <- names(the_dots)
 
   # ----------------------------------------------------------------------------
 
-  if (any(names(the_dots) == "newdata")) {
-    rlang::abort("Did you mean to use `new_data` instead of `newdata`?")
-  }
+  check_for_newdata(..., call = call)
 
   # ----------------------------------------------------------------------------
 
diff --git a/tests/testthat/_snaps/boost_tree_C50.md b/tests/testthat/_snaps/boost_tree_C50.md
@@ -0,0 +1,8 @@
+# submodel prediction
+
+    Code
+      multi_predict(class_fit, newdata = wa_churn[1:4, vars], trees = 4, type = "prob")
+    Condition
+      Error in `multi_predict()`:
+      ! Please use `new_data` instead of `newdata`.
+
diff --git a/tests/testthat/_snaps/boost_tree_xgboost.md b/tests/testthat/_snaps/boost_tree_xgboost.md
@@ -1,3 +1,11 @@
+# submodel prediction
+
+    Code
+      multi_predict(class_fit, newdata = wa_churn[1:4, vars], trees = 5, type = "prob")
+    Condition
+      Error in `multi_predict()`:
+      ! Please use `new_data` instead of `newdata`.
+
 # interface to param arguments
 
     ! Please supply elements of the `params` list argument as main arguments to `set_engine()` rather than as part of `params`.
diff --git a/tests/testthat/_snaps/linear_reg.md b/tests/testthat/_snaps/linear_reg.md
@@ -15,3 +15,11 @@
       Computational engine: glmnet 
       
 
+# newdata error trapping
+
+    Code
+      predict(res_xy, newdata = hpc[1:3, num_pred])
+    Condition
+      Error in `predict()`:
+      ! Please use `new_data` instead of `newdata`.
+
diff --git a/tests/testthat/_snaps/mars.md b/tests/testthat/_snaps/mars.md
@@ -14,3 +14,11 @@
       Computational engine: earth 
       
 
+# submodel prediction
+
+    Code
+      multi_predict(reg_fit, newdata = mtcars[1:4, -1], num_terms = 5)
+    Condition
+      Error in `multi_predict()`:
+      ! Please use `new_data` instead of `newdata`.
+
diff --git a/tests/testthat/_snaps/misc.md b/tests/testthat/_snaps/misc.md
@@ -124,3 +124,11 @@
       Error in `set_engine()`:
       ! `set_engine()` expected a model specification to be supplied to the `object` argument, but received a(n) `data.frame` object.
 
+# check_for_newdata points out correct context
+
+    Code
+      fn(newdata = "boop!")
+    Condition
+      Error in `fn()`:
+      ! Please use `new_data` instead of `newdata`.
+
diff --git a/tests/testthat/test_boost_tree_C50.R b/tests/testthat/test_boost_tree_C50.R
@@ -121,9 +121,8 @@ test_that('submodel prediction', {
   mp_res <- do.call("rbind", mp_res$.pred)
   expect_equal(mp_res[[".pred_No"]], pred_class[, "No"], ignore_attr = TRUE)
 
-  expect_error(
-    multi_predict(class_fit, newdata = wa_churn[1:4, vars], trees = 4, type = "prob"),
-    "Did you mean"
+  expect_snapshot(error = TRUE,
+    multi_predict(class_fit, newdata = wa_churn[1:4, vars], trees = 4, type = "prob")
   )
 })
 
diff --git a/tests/testthat/test_boost_tree_xgboost.R b/tests/testthat/test_boost_tree_xgboost.R
@@ -273,9 +273,8 @@ test_that('submodel prediction', {
   mp_res <- do.call("rbind", mp_res$.pred)
   expect_equal(mp_res[[".pred_Yes"]], pred_class)
 
-  expect_error(
-    multi_predict(class_fit, newdata = wa_churn[1:4, vars], trees = 5, type = "prob"),
-    "Did you mean"
+  expect_snapshot(error = TRUE,
+    multi_predict(class_fit, newdata = wa_churn[1:4, vars], trees = 5, type = "prob")
   )
 })
 
diff --git a/tests/testthat/test_linear_reg.R b/tests/testthat/test_linear_reg.R
@@ -306,7 +306,9 @@ test_that('newdata error trapping', {
     y = hpc$input_fields,
     control = ctrl
   )
-  expect_error(predict(res_xy, newdata = hpc[1:3, num_pred]), "Did you mean")
+  expect_snapshot(error = TRUE,
+    predict(res_xy, newdata = hpc[1:3, num_pred])
+  )
 })
 
 test_that('show engine', {
diff --git a/tests/testthat/test_mars.R b/tests/testthat/test_mars.R
@@ -189,9 +189,8 @@ test_that('submodel prediction', {
   mp_res <- do.call("rbind", mp_res$.pred)
   expect_equal(mp_res[[".pred_No"]], pruned_cls_pred)
 
-  expect_error(
-    multi_predict(reg_fit, newdata = mtcars[1:4, -1], num_terms = 5),
-    "Did you mean"
+  expect_snapshot(error = TRUE,
+    multi_predict(reg_fit, newdata = mtcars[1:4, -1], num_terms = 5)
   )
 })
 
diff --git a/tests/testthat/test_misc.R b/tests/testthat/test_misc.R
@@ -181,3 +181,10 @@ test_that('set_engine works as a generic', {
   )
 
 })
+
+test_that('check_for_newdata points out correct context', {
+  fn <- function(...) {check_for_newdata(...); invisible()}
+  expect_snapshot(error = TRUE,
+    fn(newdata = "boop!")
+  )
+})

Original file line number	Diff line number	Diff line change
`@@ -22,6 +22,7 @@ multi_predict <- function(object, ...) {`
`22`	`22`	`rlang::warn("Model fit failed; cannot make predictions.")`
`23`	`23`	`return(NULL)`
`24`	`24`	`}`
	`25`	`+ check_for_newdata(...)`
`25`	`26`	`UseMethod("multi_predict")`
`26`	`27`	`}`
`27`	`28`
Original file line number	Diff line number	Diff line change
`@@ -483,3 +483,13 @@ check_case_weights <- function(x, spec) {`
`483`	`483`	`}`
`484`	`484`	`invisible(NULL)`
`485`	`485`	`}`
	`486`	`+`
	`487`	`+# -----------------------------------------------------------------------------`
	`488`	`+check_for_newdata <- function(..., call = rlang::caller_env()) {`
	`489`	`+ if (any(names(list(...)) == "newdata")) {`
	`490`	`+ rlang::abort(`
	`491`	+ "Please use `new_data` instead of `newdata`.",
	`492`	`+ call = call`
	`493`	`+ )`
	`494`	`+ }`
	`495`	`+}`