-
Notifications
You must be signed in to change notification settings - Fork 88
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
remove new_data argument from predict.censoring_model_reverse_km() #1026
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
predict.censoring_model_reverse_km()
already went to CRAN so typically we'd deprecate instead of directly remove.
It's pretty obscure so I'd say a deprecate_stop()
is okay instead of kicking things off with deprecate_warn()
.
If we're not using the dots, we should check that they are empty. But I don't mind if we make that another issue 🗒️
I switched to library(censored)
mod_fit <-
survival_reg() %>%
fit(Surv(time, status) ~ age + sex, data = lung)
pred_times <- (7:10) * 100
pred_df <- predict(mod_fit$censor_probs, time = pred_times, new_data = lung)
#> Error:
#> ! The `new_data` argument of `predict.censoring_model_reverse_km()` was
#> deprecated in parsnip 1.2.0 and is now defunct. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
💯 thank you! Could you add a NEWS bullet on this before merging?
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. |
To close #965
I removed
new_data
from the function signature since it is swalloed up by...
anyways. I also added a little warning, since usingnew_data
for predict methods are quite a common actionCreated on 2023-11-17 with reprex v2.0.2