Closed mattwarkentin closed 2 years ago
Things I've noticed so far...
summary
with tidy = FALSE
returns a list predictions grouped by covariate pattern (i.e. shared patterns are grouped together); with tidy = TRUE
it may return a DF with more rows than newdata
. We need to make sure the predictions are in the exact same order as newdata
and the same number of rows as this is the tidy predict principleRelevant from docs:
If tidy=FALSE, a list with one component for each unique covariate value
If tidy=TRUE, a data frame is returned instead. This is formed by stacking the above list components
I don't actually think the second point is true, tidy = TRUE
isn't simply a bind of the list components, as far as I can tell...
Evidence:
library(flexsurv)
#> Loading required package: survival
model <- flexsurvreg(Surv(time, status) ~ age + sex, data = cancer, dist = 'weibull')
not_tidy <- summary(model, newdata = cancer, type = 'survival', t = 100, tidy = FALSE)
tidy <- summary(model, newdata = cancer, type = 'survival', t = 100, tidy = TRUE)
bind_list <- purrr::map_dfr(not_tidy, I)
all(tidy$est == bind_list$est)
#> [1] FALSE
The order of estimates is different between the two approaches so I don't think tidy = TRUE
is simply a row binding of the data-frame list components for tidy = FALSE
. Am I wrong, @chjackson ??
Okay, so I think this boils down to the fact that the expected return value from summary.flexsurvreg(tidy = FALSE)
has changed with this rewrite. I believe I now have a fix in place that has predict.flexsurvreg
working again. Just need to add some more tests to protect against this going unnoticed. Will work on a PR.
Hi @chjackson,
I think the
predict.flexsurvreg
method might be somewhat broken with the recentsummary.flexsurvreg
rewrite (98f172544755ff830c38fe772a00439844da7dde). I am going to use this thread to document the issues I've noticed and then I can work on a PR that links out to this issue.I think https://github.com/tidymodels/censored/issues/152 is related, so maybe this will solve multiple issues.