therneau / survival

Survival package for R
394 stars 106 forks source link

brier with start-stop times failed due to model.extract of "(id)" column #247

Closed Shaunson26 closed 7 months ago

Shaunson26 commented 8 months ago

In examining the brier() function with start-stop data, there seems to be an issue with model.extract() and the column named "(id)". It appears model.extract() ignores the brackets in column names? Thus to select the column "(id)", one only needs to use model.extract(frame, "id"). Is this expected?

# Survival 3.5.8
library(survival)

# use status==0 as 1, otherwise unrelated error thrown ("delayed entry is not yet implemented" if using status as-is in cgd)
fit1 <-
  survival::coxph(Surv(tstart, tstop, status==0) ~ treat, data = cgd, id = cgd$id)

# error "id is required for start-stop data" yet we have included it ...
cgd_brier <-
  survival::brier(fit1, times = seq(0, 400, by = 25))

# Seems to be an issue with model.extract for column "(id)" within brier()
mf <- stats::model.frame(fit1)
id <- model.extract(mf, "(id)")
is.null(id) # TRUE
id <- model.extract(mf, "id")
is.null(id) # FALSE
# id match
all(id == cgd$id) # TRUE

# If I alter this line in survival::brier,
# id <- model.extract(mf, "(id)")
# id <- model.extract(mf, "id")
trace(brier, edit=TRUE)

# No error and with function output
cgd_brier <-
  survival::brier(fit1, times = seq(0, 400, by = 25))

names(cgd_brier)
#[1] "rsquared" "brier"    "times" 
therneau commented 8 months ago

First: this is a bug in the code. I'll replace with (id).

Second: When R creates a variable name of its own the general rule is to add () around it. The obvious place we see this is with (Intercept) in the linear model. The same happens with model.frame for variables that are not in the formula, such as weights, etastart in glm, id in coxph and brier, etc. I suspect that the lack of () in model.extract was originally for backwards compatability with S

Third: I'm knee deep in a change to survfit, so this may wait to be bundled with that. (I'm not versed in multiple branches).

therneau commented 7 months ago

Now fixed in the latest release