Cox model: definition of timepoint

[rekkasa]

Describe the bug
If timepoint is missing from the definition of the population settings a default timepoint based on riskWindowEnd setting is set (develop branch: CyclopsModels.R, line 256). This assumes that the endAnchor setting was "cohort start". Maybe we should prevent allowing the user to set it to "cohort end" as it requires knowing a future patient's full time at risk to do predictions.

Set up (please run in R "sessionInfo()" and copy the output here):
Not needed

To Reproduce
Any cox model with timepoint = 0 and endAnchor = "cohort end" will do.

PLP Log File
Not needed

Additional context
None

[jreps]

Yeah - this was something I realized when writing the code but wasn't sure how to address it. Sometimes people want to predict from the cohort end, but then the timepoint is not possible to set as the time point is relative to cohort start. I wonder whether we can add in extra columns to the cohort where we have the time relative to the cohort end and use these times rather than those relative to the cohort start when people select cohort end? I think this would require adding a bit of SQL when extracting the data and an edit to the population code.

[egillax]

This is also an issue in predictPlp:

  # add timepoint if not missing to population attribute
  if(!missing(timepoint)){
    attr(population, 'timepoint') <- timepoint
  } else{
    timepoint <- attr(population,'metaData')$populationSettings$riskWindowEnd
  }

What about raising an error in this case:

stop("Timepoint is missing and cannot be defaulted when endAnchor is set to 'cohort end'. Please provide a valid timepoint.")

But then we need to provide a way for users of Cox to add an appropriate timepoint somehow. predictPlp does take a timepoint argument but is usually not directly called by users. Plus it's bypassed for predictions on the training set and CV predictions. Our external validation functions do call predictPlp but I don't see a way of adding a timepoint.

Would it make sense to add it to the Cox modelSettings? @rekkasa any thoughts on this ?