tidymodels / parsnip

A tidy unified interface to models
https://parsnip.tidymodels.org
Other
584 stars 88 forks source link

Deprecate `new_data` arg for `reverse_km` censoring model #965

Closed hfrick closed 9 months ago

hfrick commented 1 year ago

The predict() method for censoring_model_reverse_km models has a new_data argument which is passed on to the predict method for prodlim objects. The definition for this arg from prodlim:

A data frame with the same variable names as those that appear on the right hand side of the 'prodlim' formula.

However, the right hand side for censoring_model_reverse_km models is always just 1, no covariates: https://github.com/tidymodels/parsnip/blob/2a8f92e1a12638c8b2dfe8e47fb56923c752828a/R/survival-censoring-model.R#L27

Because of that, the argument doesn't do anything, you can even feed it a totally unrelated dataset and it won't complain. Do we anticipate adding covariates to that reverse KM anytime soon/ever? If not, can we deprecate that arg to reduce complexity we are not even making use of?

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

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)

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

hfrick commented 1 year ago

Chatted with Max: yes to covariates for the reverse KM in general but because those could be their own set of covariates (different from those on the even time distribution) adding them is complex. So, until we do that, we might as well remove/deprecate this argument.

github-actions[bot] commented 9 months ago

This issue 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.