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

Deprecate new_data arg for reverse_km censoring model #965

Closed
hfrick opened this issue May 11, 2023 · 2 comments · Fixed by #1026
Closed

Deprecate new_data arg for reverse_km censoring model #965

hfrick opened this issue May 11, 2023 · 2 comments · Fixed by #1026
Assignees
Labels
upkeep maintenance, infrastructure, and similar

Comments

@hfrick
Copy link
Member

hfrick commented May 11, 2023

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:

km_form <- stats::update(f, ~ 1)

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
Copy link
Member Author

hfrick commented May 15, 2023

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.

Copy link

github-actions bot commented Dec 5, 2023

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.

@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
upkeep maintenance, infrastructure, and similar
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants