tidymodels / tune

Tools for tidy parameter tuning
https://tune.tidymodels.org
Other
279 stars 42 forks source link

throw warning when eval_time isn't needed #704

Closed topepo closed 9 months ago

topepo commented 1 year ago

These functions should warn (but not error) when eval_time is not NULL in the following cases:

For non-survival models and for survival models with a static metric

For non-survival models and for survival models without a dynamic metric

And bringing it all together: https://github.com/tidymodels/tune/issues/811

And the exception, for consistency within the function (with how the choice of metric is treated)


show_best() should throw a warning if eval_time is used but the metric selected is not dynamic. See last line:

library(tidymodels)
library(censored)
#> Loading required package: survival

tidymodels_prefer()
theme_set(theme_bw())
options(pillar.advice = FALSE, pillar.min_title_chars = Inf)

data("mlc_churn")

mlc_churn <-
  mlc_churn %>%
  mutate(
    churned = ifelse(churn == "yes", 1, 0),
    event_time = Surv(account_length, churned)
  ) %>%
  select(event_time, account_length, voice_mail_plan) %>%
  slice(1:500)

set.seed(6941)
churn_split <- initial_split(mlc_churn, prop = 4/5)
churn_tr <- training(churn_split)
churn_te <- testing(churn_split)
churn_rs <- bootstraps(churn_tr, times = 2)

sr_tune_spec <- survival_reg(dist = tune())
eval_times <- c(10, 100, 150)
event_metrics <- metric_set(brier_survival, brier_survival_integrated,
                            concordance_survival, roc_auc_survival)
sr_grid <- tibble(dist = c("loglogistic", "lognormal"))

sr_tune_res <-
  sr_tune_spec %>%
  tune_grid(
    event_time ~ .,
    resamples = churn_rs,
    metrics = event_metrics,
    eval_time = eval_times,
    grid = sr_grid,
    control = control_grid(save_pred = TRUE)
  )

# Eval time isn't used and we should throw a warning
show_best(sr_tune_res, metric = "brier_survival_integrated", eval_time = 10)
#> # A tibble: 2 × 8
#>   dist        .metric         .estimator .eval_time   mean     n std_err .config
#>   <chr>       <chr>           <chr>           <dbl>  <dbl> <int>   <dbl> <chr>  
#> 1 loglogistic brier_survival… standard           NA 0.0282     2 0.00591 Prepro…
#> 2 lognormal   brier_survival… standard           NA 0.0399     2 0.00479 Prepro…

Created on 2023-07-20 with reprex v2.0.2

hfrick commented 11 months ago

Same for select_best(), select_by_one_std_err() and select_by_pct_loss().

Also applies when a static metric is used.

Surfaced in https://github.com/tidymodels/tune/pull/703 - those tests may need updating when closing this issue.

hfrick commented 11 months ago

Also to look into: autoplot() and int_pctl().

hfrick commented 11 months ago

The tuning functions and fit_resamples() currently error when supplying eval_time for a static survival metric and for non-censored-regression models in general. Should they also warn instead?

Here's a reprex with tune_grid() as one of the tuning functions but this should then also apply to tune_bayes() and the finetune functions.

library(tidymodels)
library(censored)
#> Loading required package: survival

# mode is not censored regression -----------------------------------------

set.seed(2193)
tune_res <-
  linear_reg(penalty = tune(), engine = "glmnet") %>%
  tune_grid(
    mpg ~ .,
    resamples = vfold_cv(mtcars, 2),
    grid = grid,
    metrics = metric_set(rmse),
    eval_time = 10
  )
#> Error:
#> ! Evaluation times are only used for dynamic and integrated survival metrics.
#> Backtrace:
#>     ▆
#>  1. ├─linear_reg(penalty = tune(), engine = "glmnet") %>% ...
#>  2. ├─tune::tune_grid(...)
#>  3. └─tune:::tune_grid.model_spec(...)
#>  4.   ├─tune::tune_grid(...)
#>  5.   └─tune:::tune_grid.workflow(...)
#>  6.     └─tune:::tune_grid_workflow(...)
#>  7.       └─tune:::check_eval_time(eval_time, metrics)
#>  8.         └─rlang::abort(...)

set.seed(2193)
tune_res <-
  linear_reg(penalty = 0.1, engine = "glmnet") %>%
  fit_resamples(
    mpg ~ .,
    resamples = vfold_cv(mtcars, 2),
    metrics = metric_set(rmse),
    eval_time = 10
  )
#> Error:
#> ! Evaluation times are only used for dynamic and integrated survival metrics.
#> Backtrace:
#>     ▆
#>  1. ├─linear_reg(penalty = 0.1, engine = "glmnet") %>% ...
#>  2. ├─tune::fit_resamples(...)
#>  3. └─tune:::fit_resamples.model_spec(...)
#>  4.   ├─tune::fit_resamples(...)
#>  5.   └─tune:::fit_resamples.workflow(...)
#>  6.     └─tune:::resample_workflow(...)
#>  7.       └─tune:::tune_grid_workflow(...)
#>  8.         └─tune:::check_eval_time(eval_time, metrics)
#>  9.           └─rlang::abort(...)

# static metric -----------------------------------------------------------

lung_surv <- lung %>% 
  dplyr::mutate(surv = Surv(time, status), .keep = "unused")

set.seed(2193)
tune_res <-
  proportional_hazards(penalty = tune(), engine = "glmnet") %>%
  tune_grid(
    surv ~ .,
    resamples = vfold_cv(lung_surv, 2),
    grid = grid,
    metrics = metric_set(concordance_survival),
    eval_time = 10
  )
#> Error:
#> ! Evaluation times are only used for dynamic and integrated survival metrics.
#> Backtrace:
#>     ▆
#>  1. ├─... %>% ...
#>  2. ├─tune::tune_grid(...)
#>  3. └─tune:::tune_grid.model_spec(...)
#>  4.   ├─tune::tune_grid(...)
#>  5.   └─tune:::tune_grid.workflow(...)
#>  6.     └─tune:::tune_grid_workflow(...)
#>  7.       └─tune:::check_eval_time(eval_time, metrics)
#>  8.         └─rlang::abort(...)

set.seed(2193)
tune_res <-
  proportional_hazards(penalty = 0.1, engine = "glmnet") %>%
  fit_resamples(
    surv ~ .,
    resamples = vfold_cv(lung_surv, 2),
    metrics = metric_set(concordance_survival),
    eval_time = 10
  )
#> Error:
#> ! Evaluation times are only used for dynamic and integrated survival metrics.
#> Backtrace:
#>     ▆
#>  1. ├─... %>% ...
#>  2. ├─tune::fit_resamples(...)
#>  3. └─tune:::fit_resamples.model_spec(...)
#>  4.   ├─tune::fit_resamples(...)
#>  5.   └─tune:::fit_resamples.workflow(...)
#>  6.     └─tune:::resample_workflow(...)
#>  7.       └─tune:::tune_grid_workflow(...)
#>  8.         └─tune:::check_eval_time(eval_time, metrics)
#>  9.           └─rlang::abort(...)

Created on 2023-11-22 with reprex v2.0.2

topepo commented 11 months ago

I think that it should be a warning. It is unneeded but won't cause bad results.

github-actions[bot] commented 8 months ago

This issue 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.