search

tidymodels / censored

Parsnip wrappers for survival models
https://censored.tidymodels.org/
Other
123 stars 12 forks source link

Try to get around usage of `time` arg in post-hook #236

Closed hfrick closed 10 months ago

hfrick commented 1 year ago

Patterns like this https://github.com/tidymodels/censored/blob/f7309a5805f66d892ac33ee7758b4f25ff7f568b/R/decision_tree-data.R#L71-L74

necessitate this in parsnip: https://github.com/tidymodels/parsnip/blob/0b6b85fbae972a489638887a15787c5ea4e9c623/R/predict_survival.R#L23-L24

Maybe we can find a way around this pattern. It causes a need for a wrapper function survival_prob_orsf() around aorsf::predict.orsf_fit() because this predict method requires empty dots (and parsnip puts the time arg into the dots).

If we do, we can remove the lines in parsnip and use the predict() method for orsf_fit objects directly and simplify/remove https://github.com/tidymodels/censored/blob/f7309a5805f66d892ac33ee7758b4f25ff7f568b/R/rand_forest-aorsf.R#L14-L31

hfrick commented 1 year ago

In addition to the usage for decision_tree() with the rpart engine mentioned above, it would also be used for the aorsf engine for rand_forest() if we would just call the predict method by the engine package directly.

For similar reformatting of the predictions, see also https://github.com/tidymodels/censored/blob/f7309a5805f66d892ac33ee7758b4f25ff7f568b/R/boost_tree-mboost.R#L123-L134

Other changes necessary: we need to explicitly pass on the time arg for survival_reg(engine = "survival") here https://github.com/tidymodels/censored/blob/f7309a5805f66d892ac33ee7758b4f25ff7f568b/R/survival_reg-data.R#L120-L124

hfrick commented 1 year ago

Note: also check the functions for predictions of type = "hazard" since parsnip has also this:

https://github.com/tidymodels/parsnip/blob/095499d62e6162f465e19f463c19c3bc1a70582c/R/predict_hazard.R#L23-L24

hfrick commented 10 months ago

If we remove adding the argument in parsnip::predict_survival.model_fit() and parsnip::predict_hazard.model_fit(), we would not be able to push formatting into the post hook because we need the values of eval_time there, not just the length. This means that this comment is then in correct and should be removed: https://github.com/tidymodels/censored/blob/d8b30c0429809c1dec5de33fd91828c0c99f6c1b/R/rand_forest-aorsf.R#L24-L26

We should still remove the argument injection in parsnip though, it just feels really hacky.

github-actions[bot] commented 10 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.