Add possibility for left-truncation to ppc_km_overlay() and ppc_km_overlay_grouped() #347
Conversation
There was a problem hiding this comment.
Thank you for the pull request! I made one comment on the code about adding documentation for the new argument. In addition to that I'm noticing a few issues with new warnings and errors in the tests for ppc_km_overlay and ppc_km_overlay_grouped. In the file tests/testthat/test-ppc-censoring.R all of the examples tested now have a warning:
Warning message:
In survival::Surv(time = data$left_trunc, time2 = data$value, event = data$group) :
Stop time must be > start time, NA created
Also, all of the "visual tests" that test that the plot doesn't look different are failing, indicating that the functions produce different plots now even when left_truncation_y is NULL.
Any idea what's going on?
Thanks again for the PR, it's great to have new contributors!
|
Thanks! Sorry for the delay in getting back to you, I've been traveling. I'll be able to take a look again hopefully tomorrow (or possibly later this week). |
There was a problem hiding this comment.
Sorry again for the delay in looking at this again. This looks great. Thanks for adding the tests. I made a few small commits:
- the random number generation in the test code was affecting some of the data for other tests unrelated to this, which wasn't your fault
- I added a test for
left_truncation_yerror message - I also took this opportunity to add tests for the
status_yerror messages, which I noticed we didn't have before
I think this is basically ready to be merged now, except there's just one thing I just noticed that may or may not be an issue. I get a warning when I run the example you added to the documentation:
> # With left-truncation (delayed entry) times:
> left_truncation_y <- runif(length(y), min = 0, max = 0.6) * y
>
> ppc_km_overlay(y, yrep[1:25, ], status_y = status_y,
+ left_truncation_y = left_truncation_y)
Warning message:
In survival::Surv(time = left_truncation_y[data$y_id], time2 = data$value, :
Stop time must be > start time, NA created
Is that to be expected?
R/ppc-censoring.R
Outdated
| #' # With left-truncation (delayed entry) times: | ||
| #' left_truncation_y <- runif(length(y), min = 0, max = 0.6) * y | ||
| #' \donttest{ | ||
| #' ppc_km_overlay(y, yrep[1:25, ], status_y = status_y, | ||
| #' left_truncation_y = left_truncation_y) | ||
| #' } |
There was a problem hiding this comment.
I get a warning when I try this example:
Warning message:
In survival::Surv(time = left_truncation_y[data$y_id], time2 = data$value, :
Stop time must be > start time, NA created
There was a problem hiding this comment.
The warning was to be expected in case of that example, but I made a cleaner example that does not produce warnings. In addition, I think the new example visualizes the effect of left-truncation a bit better.
There was a problem hiding this comment.
In real life, the warning is an indication of an impossible observation or posterior predictive draw, so the warning itself is good. When creating examples, we just have to make sure that each value in left_truncation_y is lower than the corresponding value in y or corresponding values in yrep so we don't get the warning.
There was a problem hiding this comment.
Ok great, thanks, that makes sense.
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #347 +/- ##
=======================================
Coverage 98.60% 98.60%
=======================================
Files 35 35
Lines 5533 5539 +6
=======================================
+ Hits 5456 5462 +6
Misses 77 77 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
|
@jgabry Before you merge, I think you should generate a new .Rd file for PPC-censoring, since I changed the example in my last commit. I didn't think of that yesterday. |
Will do. And actually I just noticed one more issue. The object |
|
Merging now, thanks again! |
Fixes #290