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 eval_time argument to augment.tune_results #763

Closed
wants to merge 3 commits into from

Conversation

EmilHvitfeldt
Copy link
Member

@EmilHvitfeldt EmilHvitfeldt commented Nov 20, 2023

To close #762. Tests will appear in extratests and will be coming shortly

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

set.seed(1)
sim_dat <- prodlim::SimSurv(500) %>%
  mutate(event_time = Surv(time, event)) %>%
  select(event_time, X1, X2) %>%
  as_tibble()

set.seed(2)
split <- initial_split(sim_dat)
sim_tr <- training(split)
sim_te <- testing(split)
sim_rs <- vfold_cv(sim_tr)

time_points <- c(10, 1, 5, 15)

mod_spec <-
  proportional_hazards(penalty = tune(), mixture = 1) %>%
  set_engine("glmnet") %>%
  set_mode("censored regression")

grid <- tibble(penalty = 10^c(-4, -2, -1))

gctrl <- control_grid(save_pred = TRUE)

mix_mtrc <- metric_set(brier_survival, brier_survival_integrated, concordance_survival)

set.seed(2193)
tune_res <-
  mod_spec %>%
  tune_grid(
    event_time ~ X1 + X2,
    resamples = sim_rs,
    grid = grid,
    metrics = mix_mtrc,
    eval_time = time_points,
    control = gctrl
  )

augment(tune_res)
#> Warning: No evaluation time was set; a value of 5 was used.
#> # A tibble: 375 × 5
#>    event_time    X1       X2 .pred            .pred_time
#>        <Surv> <dbl>    <dbl> <list>                <dbl>
#>  1  5.118685+     0  0.740   <tibble [4 × 3]>       6.59
#>  2  1.668954      1  1.25    <tibble [4 × 3]>       3.26
#>  3  5.282480+     1 -1.02    <tibble [4 × 3]>       9.44
#>  4  6.206428      1 -0.389   <tibble [4 × 3]>       6.93
#>  5  4.951299      1  0.524   <tibble [4 × 3]>       4.44
#>  6  5.126912      0  0.616   <tibble [4 × 3]>       7.01
#>  7 13.309537+     1 -0.852   <tibble [4 × 3]>       8.80
#>  8  1.703849      1  0.803   <tibble [4 × 3]>       3.88
#>  9  6.477580      0  0.00488 <tibble [4 × 3]>       9.18
#> 10  5.747290      1 -0.331   <tibble [4 × 3]>       6.88
#> # ℹ 365 more rows

augment(tune_res, eval_time = 1)
#> # A tibble: 375 × 5
#>    event_time    X1       X2 .pred            .pred_time
#>        <Surv> <dbl>    <dbl> <list>                <dbl>
#>  1  5.118685+     0  0.740   <tibble [4 × 3]>       6.32
#>  2  1.668954      1  1.25    <tibble [4 × 3]>       3.90
#>  3  5.282480+     1 -1.02    <tibble [4 × 3]>       9.20
#>  4  6.206428      1 -0.389   <tibble [4 × 3]>       7.22
#>  5  4.951299      1  0.524   <tibble [4 × 3]>       5.04
#>  6  5.126912      0  0.616   <tibble [4 × 3]>       6.66
#>  7 13.309537+     1 -0.852   <tibble [4 × 3]>       8.69
#>  8  1.703849      1  0.803   <tibble [4 × 3]>       4.51
#>  9  6.477580      0  0.00488 <tibble [4 × 3]>       8.34
#> 10  5.747290      1 -0.331   <tibble [4 × 3]>       7.14
#> # ℹ 365 more rows

@EmilHvitfeldt EmilHvitfeldt requested a review from hfrick November 20, 2023 23:58
Copy link
Member

@hfrick hfrick left a comment

Choose a reason for hiding this comment

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

That warning sure ain't pretty! 👀

Given that we make the choice of which metric to use (the first one), I suggest that we treat eval_time the same and make a choice (and document properly) rather than expose the argument.

As to what choice to make:
I've opened #766 to make it a consistent choice for all situations where we need to pick a single value for eval_time. Currently looks like "the first one" is the way to go but Max wanted to have a look at this as well.

@topepo
Copy link
Member

topepo commented Dec 18, 2023

ok., Weird. It was doing a conflict for the description file that had main being much smaller 9000 number than I thought.

When any of us change this, please revise the version a little higher than main. If this breaks any tests, we'd like to have a good option to skip.

@hfrick
Copy link
Member

hfrick commented Dec 18, 2023

I think we are going to close this PR anyway because augment.tune_results() is not going to get an eval_time argument to pass on to select_best(). Instead we are going to make that choice ourselves, in the same sense that we make a choice of metric when parameters is NULL.

@topepo topepo closed this Jan 4, 2024
@hfrick hfrick deleted the augment-eval_time branch January 4, 2024 18:25
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 Jan 19, 2024
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.

augment() for censored tune_results objects doesn't let you pass eval_time to select_best()
3 participants