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

add prompt for params that will not be tuned #549

Merged
merged 19 commits into from
Oct 4, 2022
Merged

add prompt for params that will not be tuned #549

merged 19 commits into from
Oct 4, 2022

Conversation

simonpcouch
Copy link
Contributor

@simonpcouch simonpcouch commented Sep 26, 2022

Closes #548.

I came across some similar confusion to Emil's in his issue #548. That issue was originally on parsnip, but I think implementing this check in tune makes more sense, as it isn't relevant outside of parameter tuning and we'd run into the same challenges as we do with other parsnip model spec checking, where it's not immediately clear when a user is "done" defining a model specification.

[EDIT: this PR originally made changes to both the existing warning and added a new one. now, it only adds a new error.]

library(tidymodels)

The error flags when a user marks parameters for tuning when they can’t be tuned due to choice of engine.

tune_grid(
  linear_reg(engine = "lm", penalty = tune()),
  mpg ~ .,
  bootstraps(mtcars, 2)
)
#> Error in `tune_grid()`:
#> ! The parameter `penalty` was marked with `tune()`, though will not be
#>   tuned.
#> ℹ This usually means that the current modeling engine `lm` does not support
#>   tuning `penalty`.

tune_bayes(
  linear_reg(engine = "lm", penalty = tune(), mixture = tune()),
  recipe(mpg ~ ., mtcars) %>% step_ns(hp, deg_free = tune()),
  bootstraps(mtcars, 2)
)
#> Error in `tune_bayes()`:
#> ! The parameters `penalty` and `mixture` were marked with `tune()`,
#>   though will not be tuned.
#> ℹ This usually means that the current modeling engine `lm` does not support
#>   tuning `penalty` and `mixture`.

Note that this PR does not change either the conditions or content of the warning shown in the original issue. That warning is raised to address a different issue, and this error catches the cases that that warning erroneously was made to handle. :)

tune_grid(
  linear_reg(),
  mpg ~ .,
  bootstraps(mtcars, 2)
)
#> Warning: No tuning parameters have been detected, performance will be evaluated
#> using the resamples with no tuning. Did you want to [tune()] parameters?
#> # Tuning results
#> # Bootstrap sampling 
#> # A tibble: 2 × 4
#>   splits          id         .metrics         .notes          
#>   <list>          <chr>      <list>           <list>          
#> 1 <split [32/11]> Bootstrap1 <tibble [2 × 4]> <tibble [0 × 3]>
#> 2 <split [32/13]> Bootstrap2 <tibble [2 × 4]> <tibble [0 × 3]>

See test cases for confirmation that we don’t raise it when we ought not to, incl. compatibility with recipes and bayesian tuning. Comment below addresses why this is an error rather than a warning.

Created on 2022-09-30 by the reprex package (v2.0.1)

@simonpcouch
Copy link
Contributor Author

This should also help us immediately surface bugs like tidymodels/rules#49, tidymodels/bonsai#30, and others as they come up in the future.

@simonpcouch simonpcouch requested review from topepo and removed request for topepo September 27, 2022 16:13
@DavisVaughan

This comment was marked as resolved.

@DavisVaughan

This comment was marked as resolved.

tests/testthat/test-checks.R Outdated Show resolved Hide resolved
R/checks.R Outdated Show resolved Hide resolved
R/checks.R Outdated Show resolved Hide resolved
R/checks.R Outdated Show resolved Hide resolved
R/checks.R Outdated Show resolved Hide resolved
topepo and others added 2 commits September 30, 2022 09:06
Co-authored-by: Davis Vaughan <davis@rstudio.com>
slightly more granular, but still not as exact as testing with dev `expect_no_warning` with a class
@simonpcouch
Copy link
Contributor Author

simonpcouch commented Sep 30, 2022

Making a few changes here at the moment, but a proposal: I think we may want to elevate the first warning to an error.

[EDIT: the first warning is now an error, and the two are thus not ever displayed one after the other]

tune::tune_grid(
    parsnip::linear_reg(penalty = tune(), mixture = tune()),
    mpg ~ .,
    rsample::bootstraps(mtcars, 2)
)
#> Warning: ! The parameters `penalty` and `mixture` were marked with `tune()`, though will
#>   not be tuned.
#> ℹ This usually means that the current modeling engine `lm` does not support
#>   tuning `penalty` and `mixture`.
#> Warning: ! No tuning parameters have been detected; performance will be evaluated using
#>   the resamples with no tuning.
#> # Tuning results
#> # Bootstrap sampling 
#> # A tibble: 2 × 4
#>   splits          id         .metrics         .notes          
#>   <list>          <chr>      <list>           <list>          
#> 1 <split [32/13]> Bootstrap1 <tibble [2 × 4]> <tibble [0 × 3]>
#> 2 <split [32/10]> Bootstrap2 <tibble [2 × 4]> <tibble [0 × 3]>

Created on 2022-09-30 by the reprex package (v2.0.1)

If a user supplies parameters for tuning, and sees the first warning, they will almost surely rerun this code. Fitting to resamples seems like a costly fallback, especially since the warning is only thrown when the results are returned, i.e. after the user has waiting for fitting to resamples.

If a user doesn't supply parameters for tuning but calls tune::tune_grid, I think it's fine if we continue to warn and fallback to fitting resamples. I believe was the original intention of scope for the second warning and I see its usefulness.

This would be breaking for code that relies on that first kind of fallback, that we unintentionally supported, which surely exists in some places, though I imagine not in package code.

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.
@simonpcouch simonpcouch changed the title add warning for params that will not be tuned add prompt for params that will not be tuned Sep 30, 2022
Copy link
Contributor Author

@simonpcouch simonpcouch left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This PR should be ready to go. :)

R/checks.R Outdated Show resolved Hide resolved
R/checks.R Show resolved Hide resolved
@topepo topepo merged commit 08fc285 into main Oct 4, 2022
@topepo topepo deleted the will-not-tune branch October 4, 2022 17:03
@github-actions
Copy link

This pull request has been automatically locked. If you believe you have found a related problem, please file a new issue (with a reprex: https://reprex.tidyverse.org) and link to this issue.

@github-actions github-actions bot locked and limited conversation to collaborators Oct 19, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

No error/warning for mismatch between model arguments and set_engine()
3 participants