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 #936

Merged
merged 2 commits into from
Mar 23, 2023
Merged

time to eval_time #936

merged 2 commits into from
Mar 23, 2023

Conversation

hfrick
Copy link
Member

@hfrick hfrick commented Mar 22, 2023

Closes #892

This PR deprecates the time argument to the prediction functions in favor of the eval_time argument name.

  • predict() only takes this argument in the ellipsis, so I updated only the documentation. I replaced time with eval_time, i.e. there is no mention of any deprecation but instead a direct swap. Should this talk about deprecation as well? I was trying not to overload that help page since it contains so much already.
  • predict_survival() and predict_hazard() both had a time argument. Now they have both time and eval_time as arguments, and time is deprecated, including updates to the documentation.

This PR starts the deprecation at deprecate_warn() rather than a soft-deprecation. Does that sound good?

Tests on this deprecation are going to have to sit in censored since we need an engine for those prediction types. parsnip does contain surv_reg(), but that uses the mode "regression" for which we don't predict survival or hazard probabilities.

R/predict.R Show resolved Hide resolved
@EmilHvitfeldt
Copy link
Member

Should this talk about deprecation as well?

I don't think it would be needed. We error pretty well, and it will clog up the documentation. Also, although we have been working on this for a while, it is also fairly experimental and this argument name change shouldn't be too unexpected

@topepo
Copy link
Member

topepo commented Mar 22, 2023

Can you do a quick bench::mark() on prediction with and without the new deprecation notices? I want to avoid negating any speed gains.

@hfrick
Copy link
Member Author

hfrick commented Mar 23, 2023

If you trigger the deprecation message, it is slower, as expected. If you switch to the new argument name, avoiding to trigger that warning, the speed is the same.

With main for both parsnip and censored:

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

bench::mark(
  main = fit(survival_reg(), Surv(time, status) ~ age + sex, data = lung) %>% 
    predict(lung, type = "survival", time = c(100, 200)), 
  iterations = 25
)
#> # A tibble: 1 × 6
#>   expression      min   median `itr/sec` mem_alloc `gc/sec`
#>   <bch:expr> <bch:tm> <bch:tm>     <dbl> <bch:byt>    <dbl>
#> 1 main         47.5ms   50.7ms      19.6      13MB     18.0

Created on 2023-03-23 with reprex v2.0.2

With the PRs for both parsnip and censored:

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

bench::mark(
  dev_with_warning = fit(survival_reg(), Surv(time, status) ~ age + sex, data = lung) %>%
    predict(lung, type = "survival", time = c(100, 200)),
  dev_without_warning = fit(survival_reg(), Surv(time, status) ~ age + sex, data = lung) %>% 
    predict(lung, type = "survival", eval_time = c(100, 200)), 
  iterations = 25
)
#> Warning: The `time` argument of `predict_survival()` is deprecated as of parsnip
#> 1.0.4.9005.
#> ℹ Please use the `eval_time` argument instead.
#> ℹ The deprecated feature was likely used in the parsnip package.
#>   Please report the issue at <https://github.com/tidymodels/parsnip/issues>.
#> This warning is displayed once every 8 hours.
#> Call `lifecycle::last_lifecycle_warnings()` to see where this warning was
#> generated.
#> # A tibble: 2 × 6
#>   expression               min   median `itr/sec` mem_alloc `gc/sec`
#>   <bch:expr>          <bch:tm> <bch:tm>     <dbl> <bch:byt>    <dbl>
#> 1 dev_with_warning      54.3ms   56.9ms      17.5    16.7MB     16.2
#> 2 dev_without_warning   48.2ms   49.6ms      19.6     967KB     15.4

Created on 2023-03-23 with reprex v2.0.2

@topepo topepo merged commit b64f30f into main Mar 23, 2023
@topepo topepo deleted the eval_time-rename branch March 23, 2023 13:41
@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.

Change argument name for evaulation time of predicted survival probabilities
3 participants