-
Notifications
You must be signed in to change notification settings - Fork 8
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
Enhance documention in define.R
#305
Conversation
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.
Hi @elong0527 , thanks for enriching the documentation of define_xxx
. The edits look great to me and I only have 1 minor comment below. Please feel free to comment if you have any concerns.
R/define.R
Outdated
@@ -18,6 +18,10 @@ | |||
|
|||
#' Define enrollment rate | |||
#' | |||
#' The function define subject enrollment rate for a study. |
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.
Shall we briefly introduce how define_enroll_rate
works under stratified designs?
Here is some notes from Keaven's email, which might be helpful. "Before we had a prevalence for each stratum SEPARATE from enrollment rates. This assumed there would be a set of overall piecewise enrollment rates and then a multinomial probability would be used to derive stratum randomly for each observation. The help file does not document behavior well."
Besides, could you please help explain the 2nd example of define_enroll_rate
?
# Define enroll rate with stratum
define_enroll_rate(
stratum = ("low", "low", "high"),
duration = (2, 2, 10),
rate = (3, 6, 9)
)
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.
The current define and AHR functions did not consider different prevalence in each stratum. From the source code of the AHR function. It seems only work for equal weight. Feel a different issue is required to address unequal prevalence in each stratum before we can document the details
The 2nd example has been updated, I feel the example is self explained.
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.
Hi @elong0527 , could you please check if you pushed the new edits?
define.R
define.R
Here is an example of stratified design: https://merck.github.io/gsDesign2/articles/story-ahr-under-nph.html#design-scenario-1. |
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.
I made a few comments. Please consider patching them up. I agree with @LittleBeannie that some aspects of the spec deserve better articulation.
@@ -97,11 +106,11 @@ define_enroll_rate <- function( | |||
#' | |||
#' # Define enroll rate with stratum |
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.
For both examples, please please consider explaining in plain language what are being defined - this is because something looks natural to you doesn't mean it's "self-explanatory" to all - maybe it's because you are more familiar with the spec. 🙏
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.
The doc are updated. Please take another look to see if it is clear.
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 for the updates, Yilong! I just approved this PR and will merge it after Nan's approval.
R/define.R
Outdated
@@ -18,6 +18,10 @@ | |||
|
|||
#' Define enrollment rate | |||
#' | |||
#' The function define subject enrollment rate for a study. |
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.
Hi @elong0527 , could you please check if you pushed the new edits?
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.
I appreciate the updates - I also improved a few details. Let's merge this.
Fixes #302
define_fail_rate
.