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

Survival analysis updates #88

Merged
merged 5 commits into from
Dec 13, 2023
Merged

Survival analysis updates #88

merged 5 commits into from
Dec 13, 2023

Conversation

topepo
Copy link
Member

@topepo topepo commented Dec 12, 2023

Update the tune functions here to keep up with the changes in the tune.

These are pretty straightforward changes. Testing for survival models will be in extratests (after this is merged, step 6 below)

An updated itinerary of PRs

  1. Tools for selecting a default evaluation time tune#768
  2. Refactor choose_metric tune#777
  3. Refactored functions to select single evaluation times tune#778
  4. Refactored functions to evaluate/check metric and eval time arguments tune#780
  5. (you are here) update finetune to use these changes
  6. Unit tests that can't go into tune/finetune in the extratests repo
  7. Revist some of the checking functions and messaging (e.g., Finetuning warnings for show_best() on results of fit_resamples() and other tune_results tune#781)
  8. Add additional functions that return (potentially) multiple evaluation times (eg autoplot())
  9. Find remaining unused functions in tune (like middle_eval_time()) to remove
  10. Write an article on tidymodels.org about this.

@topepo topepo marked this pull request as ready for review December 12, 2023 22:00
@topepo topepo requested a review from hfrick December 12, 2023 22:01
R/tune_race_anova.R Show resolved Hide resolved

racing_obj_log(res, metrics, control, eval_time)
eval_time <- tune::check_eval_time_arg(eval_time, metrics, call = call)
Copy link
Member

Choose a reason for hiding this comment

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

same question about the call here

Copy link
Member Author

Choose a reason for hiding this comment

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

☝️

R/tune_race_anova.R Outdated Show resolved Hide resolved
R/tune_race_anova.R Outdated Show resolved Hide resolved
@@ -387,7 +387,7 @@ harmonize_configs <- function(x, key) {
x
}

restore_tune <- function(x, y) {
restore_tune <- function(x, y, eval_time_target = NULL) {
Copy link
Member

Choose a reason for hiding this comment

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

x and y are both tune_results objects, right? do they now both carry eval_time_target as an attribute that could be used here instead of providing it as an argument?

Copy link
Member Author

Choose a reason for hiding this comment

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

x has class tune_results. y has the same structure but different attributes. The extra classes have been dropped due to row filtering of the resampling object. The function restores missing attributes to y.

However... x is from tune_grid(), which has no notion of a target evaluation time. tidymodels/tune#782 defaults eval_time_target to NULL for grid tuning, resampling, and last fit objects.

That's why eval_time_target is an argument to this function. It should be non-null for the resulting racing object but the value inherited from x is NULL unless we set it.

Copy link
Member

Choose a reason for hiding this comment

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

Ah, thanks! 👍

Copy link
Member Author

Choose a reason for hiding this comment

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

notes added to sources

topepo added a commit to tidymodels/tune that referenced this pull request Dec 13, 2023
@topepo topepo merged commit a57178b into main Dec 13, 2023
8 checks passed
@topepo topepo deleted the survival-analysis-updates branch December 13, 2023 18:57
topepo added a commit to tidymodels/tune that referenced this pull request Dec 13, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants