therneau / survival

Survival package for R
394 stars 106 forks source link

concordance with start/stop data and timewt='S' crashes R #244

Closed Shaunson26 closed 6 months ago

Shaunson26 commented 8 months ago

concordance() with start/stop data and timewt = S crashes R.

I've narrowed it done to the C script Cfastkm2.

Here is a reprex

library(survival)

fit1 <- 
  coxph(Surv(tstart,tstop, status) ~ inherit, data = cgd)

# concordance(fit1, timewt='S') # crash
# concordance() -> concordancefit() -> docount() -> Cfastkm2

y = fit1$y
wts = rep(1, length(y))
risk = fit1$linear.predictors
sort.stop <- order(-y[, 2], y[, 3], risk) - 1L
sort.start <- order(-y[, 1]) - 1L

.Call(survival:::Cfastkm2, y, wts, sort.stop, sort.start)
RobinDenz1 commented 8 months ago

I tried this out of pure curiosity and can confirm that this indeed crashes R-Studio

Shaunson26 commented 8 months ago

I also tried in R, and outside Rstudio, and it still crashes. There was once an issue with another package that crashed Rstudio but not R.

therneau commented 8 months ago

This is serious stuff. What is surprising is that I can make it happen with R sometimes, but not when I turn on valgrind, the tool that would help me find it. Working on it....

therneau commented 6 months ago
  1. You gave a great example 2. I think time weights (Uno paper) is a tempest in a teacup so don't pay much attention to it, and thus never created an example in my test suite -- big mistake. 3. There were two serious flaws, one in init.c that caused the crash (fastkm1 appeared twice), and another that gives wrong results (sort1 before sort2 in the C code, sort2 before sort1 in the call to the C code).
    Those are fixed, now working on an addition to the test suite.

Now resolved; the code has been updated