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

Enable multi_predict._coxnet() for all types #282

Merged
merged 4 commits into from
Jan 3, 2024
Merged

Conversation

hfrick
Copy link
Member

@hfrick hfrick commented Dec 20, 2023

Closes #267 and closes #277

This PR redoes multi_predict() for glmnet's Cox models (aka coxnet).

In parsnip, predict() is rather strict about which arguments it accepts. It does not, for example, accept any arguement to pass on multiple penalty values. We make multi_predict() work by leveraging the less strict predict(type = "raw") which allows us to pass on that information in opts.

In censored, we cannot use any methods from glmnet directly and instead have survival_prob_coxnet() and survival_time_coxnet() as the functions that take a fitted model and return predictions. They are capable of dealing with a single or multiple penalty values.

To get the formatting of the output right (multi_predict() should include a penatly column while predict() should not), we need the multi argument to those functions (you could use a single penalty value in either setting). However, we cannot pass that through predict(), see inital comment about its strictness.

We could either allow a special in case in parsnip or bypass predict() and call those survival_*_coxnet() functions directly from multi_predict().

Adding yet more special glmnet code to parsnip, and for survival models in particular, did not seem very appealing so I went with the second option here. It does mean that we do need to carry out some of the check that otherwise predict() would carry out: is the type of prediction available for this model spec, what's in the dots, etc.

I've copied those over from parsnip for now, rather than export them from there because I think they might change a bit when we overhaul checking functions in parsnip. So down the line, we might export them, I've just not done this yet.

I've chatted with Max about the general approach here, but if you think I overlooked something or a different approach is better, please do shout!

@hfrick hfrick requested a review from EmilHvitfeldt December 20, 2023 12:10
@hfrick
Copy link
Member Author

hfrick commented Dec 20, 2023

Note to self: add a NEWS item!

Copy link
Member

@EmilHvitfeldt EmilHvitfeldt left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it looks good!

There is a lot of "copied directly from parsnip" which would be nice to have exported, but that is not a priority right now.

@hfrick
Copy link
Member Author

hfrick commented Jan 3, 2024

Yeah, not loving the parsnip copies but hoping it's the better decision for now!

@hfrick hfrick merged commit dd0044e into main Jan 3, 2024
9 checks passed
@hfrick hfrick deleted the multi_predict-coxnet branch January 3, 2024 14:51
Copy link

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 Jan 18, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
2 participants