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
To solve issue #547.