Develop gs_cp_ahr following gsDesign::gsCP#550
Conversation
2. clean up the comments 3. fixed the details braces issue 4. update the beta the last upper bound, which should be the futility bound from a, not b
…thub.com/Merck/gsDesign2 into 547-develop-gs_cp-following-gsdesigngscp
jdblischak
left a comment
jdblischak
left a comment
There was a problem hiding this comment.
Just a few suggestions for improvements. Overall this looks good. I like the developer tests.
Also, with 28 commits made over multiple months, this PR is a good candidate for the "Squash and Merge" feature. Having a single commit for the implementation of gs_cp_npe2() makes future troubleshooting easier
| info = NULL, | ||
| a = NULL, | ||
| b = NULL, | ||
| c = NULL){ |
There was a problem hiding this comment.
These variable names are very short for a function that is exposed to end users. Are these single letters easily recognizable to practitioners, eg from a well-known formula?
If not, I would recommend giving them more informative names. You can always re-assign them to shorter names inside the function if this aids the readability of the code.
| prob_alpha_plus <- rep(0, n_future_analysis) | ||
| prob_beta <- rep(0, n_future_analysis) | ||
|
|
||
| for(x in 1:n_future_analysis){ |
There was a problem hiding this comment.
I recommend using seq_len() here as you do below
| # vector of length x | ||
| # ------------------------------ # | ||
|
|
||
| mu <- sapply(seq_len(x), function(k){ |
There was a problem hiding this comment.
vapply() is safer to use inside of a function
Connecting with my comment above about the variable names. If you converted this anonymous function to a named function, you could name its argument as t and then pass the longer variable name to it.
| # ----------------------------------- # | ||
| ans <- list() | ||
|
|
||
There was a problem hiding this comment.
Please remove this unnecessary whitespace
| \begin{table}[t] | ||
| \caption*{ | ||
| {\large Fixed Design under AHR Method\textsuperscript{\textit{1}}} | ||
| {\fontsize{20}{25}\selectfont Fixed Design under AHR Method\textsuperscript{\textit{1}}\fontsize{12}{15}\selectfont } |
There was a problem hiding this comment.
Are these changes to the snapshot test still needed now that #589 was merged?
2. Create gs_cp_npe to call gs_cp_npe1 and gs_cp_npe2
…ed gs_cp_npe1 and gs_cp_npe2 and corresponding test function
| #' \deqn{P(\{Z_j \leq b_j\} \& \{\cap_{m=i+1}^{j-1} a_m \leq Z_m < b_m\} \mid Z_i = z_i)}. | ||
| #' | ||
| #' @param x An object of type gsDesign2. | ||
| #' @param theta Optional numeric vector with length \eqn{j-i+1}, which specifies the natural parameter for treatment effect of interim analysis \eqn{i} through analysis \eqn{j}. |
There was a problem hiding this comment.
Please indicate the default value of theta, i.e., we impute the default theta = NULL as the planned treatment effect.
| #' @param theta Optional numeric vector with length \eqn{j-i+1}, which specifies the natural parameter for treatment effect of interim analysis \eqn{i} through analysis \eqn{j}. | ||
| #' @param i Index of current analysis, with default of 1. | ||
| #' @param zi Numeric scalar z-value observed at analysis \eqn{i}. | ||
| #' @return A list of conditional powers: prob_alpha is a numeric vector of (\eqn{alpha_i,i+1, ..., alpha_i,j-1, alpha_i,j}), where alpha_i,j = \eqn{P(\{Z_j \geq b_j\} \& \{\cap_{m=i+1}^{j-1} a_m \leq Z_m < b_m\} \mid Z_i = z_i)}. |
There was a problem hiding this comment.
prob_alpha --> prob_alpha
prob_alpha --> prob_alpha_plus
prob_beta --> prob_beta
| #' gs_cp(x = x, i = 1, zi = -gsDesign::hrn2z(hr = 0.8, n = 150+180, ratio = 1)) | ||
| #' | ||
| #' # case 2: currently at IA1, compute conditional power at IA2, IA3 and FA, with user-input theta | ||
| #' gs_cp(x = x, i = 1, zi = -gsDesign::hrn2z(hr = 0.8, n = 150+180, ratio = 1)) |
There was a problem hiding this comment.
It seems the case 2 is the same as case 1. Could you please check on your end?
| # You should have received a copy of the GNU General Public License | ||
| # along with this program. If not, see <http://www.gnu.org/licenses/>. | ||
|
|
||
| #' Conditional power computation with non-constant effect size |
There was a problem hiding this comment.
Conditional power computation with non-constant effect size
-->
Simple conditional power computation with non-constant effect size
| @@ -19,38 +19,34 @@ | |||
| #' Conditional power computation with non-constant effect size | |||
There was a problem hiding this comment.
Is this gs_cp_npe1 function a general calculator of simple coditional power? If so, coule you please indicate it in the title at line 19?
3 participants
To solve issue #547.