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

remove new_data argument from predict.censoring_model_reverse_km() #1026

Merged
merged 5 commits into from
Nov 20, 2023

Conversation

EmilHvitfeldt
Copy link
Member

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 using new_data for predict methods are quite a common action

library(censored)

mod_fit <-
  survival_reg() %>%
  fit(Surv(time, status) ~ age + sex, data = lung)

# can use different data (because reverse_km model uses ~ 1)
pred_newdata <- predict(mod_fit$censor_probs, time = (7:10) * 100,
                        new_data = mtcars)
#> Warning: `new_data` is ignored when predicting on `censoring_model_reverse_km()` models
#> as it doesn't affect the result.

Created on 2023-11-17 with reprex v2.0.2

@EmilHvitfeldt EmilHvitfeldt requested a review from hfrick November 17, 2023 22:34
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.

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 🗒️

@EmilHvitfeldt
Copy link
Member Author

I switched to deprecate_stop() now

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.

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.

💯 thank you! Could you add a NEWS bullet on this before merging?

@EmilHvitfeldt EmilHvitfeldt merged commit 1db4a8b into main Nov 20, 2023
4 checks passed
@EmilHvitfeldt EmilHvitfeldt deleted the fix-965 branch November 20, 2023 19:59
Copy link

github-actions bot commented Dec 5, 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 Dec 5, 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.

Deprecate new_data arg for reverse_km censoring model
2 participants