-
Notifications
You must be signed in to change notification settings - Fork 0
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
69: Function to compute empirical power for a list of simulated trials #70
Conversation
Code Coverage Summary
Diff against main
Results for commit: e28dc85 Minimum allowed coverage is ♻️ This comment has been updated with latest results |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nice work, thanks @holgstr - please see few suggestions below
R/powerEmp.R
Outdated
#' )[[1]] | ||
#' logRankTest(data = simTrial, typeEvent = "OS", critical = 3.4) | ||
logRankTest <- function(data, typeEvent, critical) { | ||
# must be `1rowTransition`, i.e. have 10 (censored) or 11 columns |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
# must be `1rowTransition`, i.e. have 10 (censored) or 11 columns | |
# Must be `1rowTransition`, i.e. have 10 (censored) or 11 columns. |
(best to use sentence style for all comments in the code)
re: what means "1rowTransition" ? would it be better to say a bit more here instead of using this abbreviation?
R/powerEmp.R
Outdated
logRankTest <- function(data, typeEvent, critical) { | ||
# must be `1rowTransition`, i.e. have 10 (censored) or 11 columns | ||
assert_data_frame(data, min.cols = 10, max.cols = 11) | ||
assert_choice(typeEvent, c("OS", "PFS")) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
instead of this it is even more elegant to use typeEvent = c("OS", "PFS")
in the function signature and then here typeEvent <- match.arg(typeEvent)
R/powerEmp.R
Outdated
event <- data$PFSevent | ||
} | ||
|
||
logRank <- survdiff(Surv(time, event) ~ trt, data) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
logRank <- survdiff(Surv(time, event) ~ trt, data) | |
logRank <- survival::survdiff(Surv(time, event) ~ trt, data) |
since it is not in base R
R/powerEmp.R
Outdated
assert_int(eventNumPFS, lower = 1L) | ||
assert_int(eventNumOS, lower = 1L) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
could also use assert_count(..., positive = TRUE)
R/powerEmp.R
Outdated
nRep <- length(simTrials) | ||
simTrials <- lapply(simTrials, function(x) if (ncol(x) == 9) getDatasetWideFormat(x) else x) | ||
|
||
# Censor simulated trials at time-point of OS/PFS analysis |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
# Censor simulated trials at time-point of OS/PFS analysis | |
# Censor simulated trials at time-point of OS/PFS analysis. |
etc (sentence style)
R/powerEmp.R
Outdated
simTrials <- lapply(simTrials, function(x) if (ncol(x) == 9) getDatasetWideFormat(x) else x) | ||
|
||
# Censor simulated trials at time-point of OS/PFS analysis | ||
trialsAnaPFS <- lapply(simTrials, censoringByNumberEvents, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
here we see repeated code for PFS and OS (3 calls which are basically the same for both) - here it would be useful to add an internal helper function that is then used twice here, once for PFS and once for OS to avoid the code duplication
R/powerEmp.R
Outdated
# Joint power: Both PFS/OS have significant log-rank tests | ||
powerJoint <- sum(sumPassed == 2) / nRep | ||
|
||
c("powerPFS" = powerPFS, "powerOS" = powerOS, "powerAtLeast" = powerAtLeast, "powerJoint" = powerJoint) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
best to line break after each element, also consider returning a list instead of a vector - but check first the rest of the package what we use more so far in this kind of situation
re: survival package - we have this currently in Suggests only (https://github.com/insightsengineering/simIDM/blob/main/DESCRIPTION#L34) I would move this to Imports and then import a single function in here: https://github.com/insightsengineering/simIDM/blob/main/R/package.R#L8 (does not matter which one, we always will prefix in our code with |
R/powerEmp.R
Outdated
eventNum = eventNumOS, typeEvent = "OS" | ||
) | ||
# Helper function to conduct log-rank tests for either PFS or OS: | ||
passedLogRank <- function(typeEvent, eventNum, critical) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this could be good, but please move it out of the powerEmp
function - because only then you can document and test separately
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
thx @danielinteractive. Can I infer from this that we want "any" function defined separately (i.e., outside the main function) if it is not just a one-liner or a very simple if-else structure (or sth. similar)?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @holgstr please see last comments below
@danielinteractive I have implemented all of your suggestions :) |
Fixes #69