Skip to content
This repository has been archived by the owner on Jun 14, 2023. It is now read-only.

IPCW article #282

Draft
wants to merge 6 commits into
base: main
Choose a base branch
from
Draft

IPCW article #282

wants to merge 6 commits into from

Conversation

topepo
Copy link
Member

@topepo topepo commented May 16, 2023

I'll leave this as draft until the other articles are also drafted.

Copy link
Member

@hfrick hfrick left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Some more small edits as suggestions 😄


- Observed time: time recorded in the data
- Event time: observed times for actual events
- Evaluation time: the time, specified by the analyst, that the model is evaluated.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

should this be "that the model is evaluated at"? or maybe "evaluated for"? 🤔

@hfrick
Copy link
Member

hfrick commented Jun 7, 2023

Todo:

  • update usage of time_as_binary_event() when exported from parsnip survival time to binary conversion parsnip#973
  • update links to the main article on performance metrics for survival models
  • update to CRAN versions (for all but censored and parsnip) before rendering

Comment on lines +109 to +110
- **Category 1 - Events**: Evaluation time is greater than or equal to the event time ("it has already happened").
- **Category 2 - Non-events**: Evaluation time is less than the observed time, censored or not ("nothing has happened yet").
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've removed the "definitive" qualifier here since we then go on to add a probability for that and that feels somewhat "off" to "definitive".

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Another important change: I've flipped the language and now describe it in terms of evaluation time relative to observed time since evaluation time is the thing we are varying and the observed time is fixed.

Comment on lines +159 to +160
geom_vline(aes(xintercept = eval_time, col = I("red"), linetype = I("dashed"), linewidth = I(0.8))) +
geom_point(aes(eval_time, obs_id, shape = eval_status, col = I("red"), size = I(5))) +
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have not figured out in justifiable time how to get the red into the legend. Accept that small mismatch or go back to all black?

not using `.time_as_binary_event()` since that requires a single value for eval_time
binary_encoding <-
dyn_val_pred %>%
mutate(
obs_class = time_as_binary_event(event_time, .eval_time),
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

.time_as_binary_event() from parsnip wants eval_time as a single numeric value, to use that it would have to be something like this:

binary_encoding <- 
  dyn_val_pred %>% 
  rowwise() %>% 
  mutate(
    obs_class = .time_as_binary_event(event_time, .eval_time),
  ) %>% 
  ungroup() %>% 
  mutate(
    pred_class = if_else(.pred_survival >= 1 / 2, "non-event", "event"),
    pred_class = factor(pred_class, levels = c("event", "non-event")),
  )

So if we want to remove the function definition here and use the parsnip version, we might want to consider tweaking that one first. The above code block feels pretty heavy on the friction.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants