From 08fc285eff5ba4f04632e2906014ce0be47453fc Mon Sep 17 00:00:00 2001 From: "Simon P. Couch" Date: Tue, 4 Oct 2022 13:03:54 -0400 Subject: [PATCH] add prompt for params that will not be tuned (#549) * implement warning for params that will not be tuned, update existing snaps * add tests for warning re: params that will not tune * add NEWS bullet * clarify dependencies * Apply suggestions from code review Co-authored-by: Davis Vaughan * `expect_silent` -> `expect_warning` with `regexp = NA` slightly more granular, but still not as exact as testing with dev `expect_no_warning` with a class * simplify conditions, rephrase prompt only check the model object, as there's not a well-defined analogue for the "engine" in recipes. this also helps simplify the cli machinery. * restore current warning, elevate new warning to error * extend testing * update NEWS * remove outdated remote * move new check into sub-function * spelling issue * check installs for workflow (not just the model object) * Revert "check installs for workflow (not just the model object)" This reverts commit 83386394fc038e69cd34c2703e250f0f495d0d6c. Co-authored-by: Max Kuhn Co-authored-by: Davis Vaughan --- DESCRIPTION | 5 ++- NEWS.md | 5 ++- R/checks.R | 28 +++++++++++++-- tests/testthat/_snaps/checks.md | 18 ++++++++++ tests/testthat/test-checks.R | 60 +++++++++++++++++++++++++++++++++ 5 files changed, 110 insertions(+), 6 deletions(-) diff --git a/DESCRIPTION b/DESCRIPTION index 759087e34..8754b8114 100644 --- a/DESCRIPTION +++ b/DESCRIPTION @@ -27,7 +27,7 @@ Imports: GPfit, hardhat (>= 1.2.0), lifecycle (>= 1.0.0), - parsnip (>= 1.0.1.9000), + parsnip (>= 1.0.2), purrr (>= 0.3.2), recipes (>= 1.0.0), rlang (>= 1.0.2), @@ -55,5 +55,4 @@ Language: en-US LazyData: true Roxygen: list(markdown = TRUE) RoxygenNote: 7.2.1.9000 -Remotes: - tidymodels/parsnip + diff --git a/NEWS.md b/NEWS.md index 290895cc5..522de4521 100644 --- a/NEWS.md +++ b/NEWS.md @@ -4,7 +4,10 @@ * The `collect_metrics()` method for racing objects was removed (and is now in the finetune package). -* The `control_bayes()` got a new argument `verbose_iter` that is used to control the verbosity of the bayesian calculations. This change means that the `verbose` argument is being passed to `tune_grid()` to control its verbosity. +* Improved prompts related to parameter tuning. When tuning parameters are supplied that are not compatible with the given engine, `tune_*()` functions will now error. (#549) + +* The `control_bayes()` got a new argument `verbose_iter` that is used to control the verbosity of the Bayesian calculations. This change means that the `verbose` argument is being passed to `tune_grid()` to control its verbosity. + * The `control_last_fit()` function gained an argument `allow_par` that defaults to `FALSE`. This change addresses failures after `last_fit()` using modeling engines that require native serialization, and we anticipate little to no increase in time-to-fit resulting from this change. (#539, tidymodels/bonsai#52) diff --git a/R/checks.R b/R/checks.R index 5389aaef5..89e53d195 100644 --- a/R/checks.R +++ b/R/checks.R @@ -297,12 +297,36 @@ check_workflow <- function(x, pset = NULL, check_dials = FALSE) { } } - mod <- extract_spec_parsnip(x) - check_installs(mod) + check_extra_tune_parameters(x) + + check_installs(hardhat::extract_spec_parsnip(x)) invisible(NULL) } +check_extra_tune_parameters <- function(x) { + mod <- hardhat::extract_spec_parsnip(x) + + to_be_tuned <- hardhat::extract_parameter_set_dials(mod) + marked_for_tuning <- generics::tune_args(mod) + + if (nrow(marked_for_tuning) > nrow(to_be_tuned)) { + not_tunable <- marked_for_tuning$name[!marked_for_tuning$name %in% to_be_tuned$name] + msg <- + c("!" = "{cli::qty(not_tunable)}The parameter{?s} {.var {not_tunable}} + {?was/were} marked with `tune()`, though will not be tuned.", + "i" = "This usually means that the current modeling engine + {.var {extract_spec_parsnip(x)$engine}} + does not support tuning {.var {not_tunable}}." + ) + + + cli::cli_abort(msg, call = rlang::caller_env(3), class = "not_tunable_error") + } + invisible(NULL) +} + + #' @export #' @keywords internal #' @rdname empty_ellipses diff --git a/tests/testthat/_snaps/checks.md b/tests/testthat/_snaps/checks.md index 325741704..8230b8e8a 100644 --- a/tests/testthat/_snaps/checks.md +++ b/tests/testthat/_snaps/checks.md @@ -84,6 +84,24 @@ Error in `tune:::check_workflow()`: ! A parsnip model is required. +# workflow objects (will not tune, tidymodels/tune#548) + + Code + tune_grid(lr_lm_1, rec_bare, rsample::bootstraps(Chicago, 2)) + Condition + Error in `tune_grid()`: + ! The parameter `penalty` was marked with `tune()`, though will not be tuned. + i This usually means that the current modeling engine `lm` does not support tuning `penalty`. + +--- + + Code + tune_bayes(lr_lm_2, rec_tune, rsample::bootstraps(Chicago, 2)) + Condition + Error in `tune_bayes()`: + ! The parameters `penalty` and `mixture` were marked with `tune()`, though will not be tuned. + i This usually means that the current modeling engine `lm` does not support tuning `penalty` and `mixture`. + # yardstick objects Code diff --git a/tests/testthat/test-checks.R b/tests/testthat/test-checks.R index 3b6058b71..ee9e8214a 100644 --- a/tests/testthat/test-checks.R +++ b/tests/testthat/test-checks.R @@ -179,6 +179,66 @@ test_that("workflow objects", { }) }) +test_that("workflow objects (will not tune, tidymodels/tune#548)", { + skip_if_not_installed("glmnet") + + # one recipe without tuning, one with: + rec_bare <- recipes::recipe(ridership ~ ., data = head(Chicago, 30)) + rec_tune <- rec_bare %>% recipes::step_ns(temp_max, deg_free = tune()) + + # well-defined: + lr_lm_0 <- parsnip::linear_reg() + + # not well-defined: + lr_lm_1 <- parsnip::linear_reg(penalty = tune()) + lr_lm_2 <- parsnip::linear_reg(penalty = tune(), mixture = tune()) + + # well-defined: + lr_glmnet_0 <- lr_lm_0 %>% set_engine("glmnet") + lr_glmnet_1 <- lr_lm_1 %>% set_engine("glmnet") + lr_glmnet_2 <- lr_lm_2 %>% set_engine("glmnet") + + # don't error when supplied tune args make sense given engine / steps + expect_error_na <- function(x) {testthat::expect_error(x, regexp = NA)} + + expect_error_na(check_workflow(workflow(rec_bare, lr_lm_0))) + expect_error_na(check_workflow(workflow(rec_bare, lr_glmnet_0))) + expect_error_na(check_workflow(workflow(rec_bare, lr_glmnet_1))) + expect_error_na(check_workflow(workflow(rec_bare, lr_glmnet_2))) + + expect_error_na(check_workflow(workflow(rec_tune, lr_lm_0))) + expect_error_na(check_workflow(workflow(rec_tune, lr_glmnet_0))) + expect_error_na(check_workflow(workflow(rec_tune, lr_glmnet_1))) + expect_error_na(check_workflow(workflow(rec_tune, lr_glmnet_2))) + + # error when supplied tune args don't make sense given engine / steps + expect_error_nt <- function(x) {testthat::expect_error(x, class = "not_tunable_error")} + + expect_error_nt(check_workflow(workflow(rec_bare, lr_lm_1))) + expect_error_nt(check_workflow(workflow(rec_bare, lr_lm_2))) + + expect_error_nt(check_workflow(workflow(rec_tune, lr_lm_1))) + expect_error_nt(check_workflow(workflow(rec_tune, lr_lm_2))) + + # ensure that error (incl `call`) is displayed as it ought to be: + expect_snapshot( + error = TRUE, + tune_grid( + lr_lm_1, + rec_bare, + rsample::bootstraps(Chicago, 2) + ) + ) + expect_snapshot( + error = TRUE, + tune_bayes( + lr_lm_2, + rec_tune, + rsample::bootstraps(Chicago, 2) + ) + ) +}) + # ------------------------------------------------------------------------------ test_that("yardstick objects", {