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

time to eval_time #244

Merged
merged 22 commits into from Mar 23, 2023
Merged

time to eval_time #244

merged 22 commits into from Mar 23, 2023

Conversation

hfrick
Copy link
Member

@hfrick hfrick commented Mar 22, 2023

Closes #241

This PR completes the deprecation of the time argument to the prediction functions in favor of the eval_time argument name laid out in tidymodels/parsnip#892.

The commits are (mostly) working through the package model/engine combination by model/engine combination, but the aggregated pattern is

  • In the -data.R files, prediction modules for types "survival" and "hazard" need to expect the eval_time arg instead of the time arg
  • The censored functions to calculate survival or hazard probabilities from a model object, e.g., survival_prob_coxph(), also make the switch and since they are exported, time is being deprecated. Since they are also @keywords internal, there are no specific deprecation tests. Happy to add those but thought that this might be a bit much.
  • The deprecation "level" is "warn" as in time to eval_time parsnip#936
  • The internal helper functions also now use eval_time where appropriate.
  • The tests for the deprecation of the time arg in parsnip::predict_survival() and parsnip::predict_hazard() are in test-survival_reg-survival.R

@hfrick hfrick requested a review from simonpcouch March 22, 2023 16:44
@hfrick
Copy link
Member Author

hfrick commented Mar 22, 2023

@simonpcouch The changes are pretty boring to read - the more challenging aspect is if I overlooked anything to add. If you could tilt your review toward that, that'd be great! 🙌

Copy link
Contributor

@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.

I’m on board with your deprecation approach—thumbs up for the unit testing comment and transition immediately to the more aggressive deprecate_warn().

I’m guessing many of these are false positives, but a few places where .time or time is still around in a way that isn’t immediately obvious to me:

.time = x$time,

.time = rep(x$time, n),

.time = 0,

next_event_time = dplyr::lead(.time),
time_interval = next_event_time - .time,

# survival_prob is length(time) x nrow(new_data) and

I believe the intent here was to test formula vs xy rather than with the two different arguments:

There are a good few places in this vignette that use time:

time = c(100, 500, 1000)

object %>%
summary(times = time, extend = TRUE) %>%
summary(times = eval_time, extend = TRUE) %>%
Copy link
Contributor

Choose a reason for hiding this comment

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

Note to self / future GitHub sleuth--times is the argument to survival::summary.survfit()

R/bag_tree-rpart.R Outdated Show resolved Hide resolved
@hfrick
Copy link
Member Author

hfrick commented Mar 23, 2023

Thank you! The .time vectors are indeed false positives. I plan on changing those when I make up my mind between calling them observed times or event times. The other ones were very helpful, thanks!

@hfrick
Copy link
Member Author

hfrick commented Mar 23, 2023

CI failures are due to changes in the new pillar, so unrelated to this PR

@hfrick hfrick merged commit 6a38f79 into main Mar 23, 2023
6 of 8 checks passed
@hfrick hfrick deleted the eval_time-rename branch March 23, 2023 14:01
@github-actions
Copy link

github-actions bot commented Apr 7, 2023

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 Apr 7, 2023
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.

Update names for evaluation time
2 participants