search

tidymodels / dials

Tools for creating tuning parameter values
https://dials.tidymodels.org/
Other
113 stars 27 forks source link

Add flexsurv survival distributions and knots parameter #195

Closed mattwarkentin closed 2 years ago

mattwarkentin commented 3 years ago

Preliminary work relevant to https://github.com/tidymodels/censored/issues/115

topepo commented 3 years ago

Should the lower range be 1L or 2L?

mattwarkentin commented 3 years ago

k = 0 is the default for flexsurv::flexsurvspline, so I went with that. But I think 0 or 1 for the lower bound could work. I tend to still have preference for 0 for consistency with flexsurv, but I'm open to changing it. Though, k = 0 effectively fits a Weibull model, you really only start to get a spline model with 1+ knots.

hfrick commented 2 years ago

Following up here from https://github.com/tidymodels/censored/issues/115: Could you change the name from knots to num_knots, please? I'm not sure what you have in mind with values_flexsurv_dist but usually we don't add another set of default values like this. We probably have to think this one through a bit more. Could you take that one out for the moment?

mattwarkentin commented 2 years ago

Could you change the name from knots to num_knots, please?

On it!

I'm not sure what you have in mind with values_flexsurv_dist but usually we don't add another set of default values like this.

I decided to include this because flexsurvreg has parametric distributions that are not part of the set of distributions for survreg. And in some cases, even among the shared distributions, the naming is different (e.g. loglogistic vs llogis). I am happy to remove this, if desired, but I do think that in the future you may wish to support all of the distributions in flexsurvreg and not just those that overlap with survreg.

topepo commented 2 years ago

I'm going to read the source literature but do you think that we should have a dials function for the scale argument? If so, what would be a good name (since scale is pretty generic)?

mattwarkentin commented 2 years ago

I proposed surv_scale (to be similar-feeling to surv_dist) in the other thread (https://github.com/tidymodels/censored/issues/115#issuecomment-969287398). I do think this is worth including as a parameter because sometimes it is desirable to fit a proportional odds vs proportional hazards model, and vice versa. I have already created this function in my local fork of dials, I was just waiting to see what you/Hannah were thinking about its inclusion/name.

I can push it now and we can continue to iterate?

topepo commented 2 years ago

I can push it now and we can continue to iterate?

Sure.

hfrick commented 2 years ago

I'm fine with surv_scale but open to changes.

It changes the link function g() between the survivor function and the spline function but given that you supply values like "hazard" or "odds" I don't think making the name echo anything related to "link function" would necessarily increase clarity of what the parameter does.

hfrick commented 2 years ago

@mattwarkentin I've spoken with Max and he pointed out that surv_scale might get people thinking about location and scale parameters. He suggests survival_link instead. Would you like to make that change? Otherwise I'm happy to do that and then merge :raised_hands:

hfrick commented 2 years ago

Thanks so much for your contribution @mattwarkentin ! Looking forward to your PR on censored!

github-actions[bot] commented 2 years ago

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.