-
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
Clean up summary
functions
#461
Conversation
…irst and second elements in x$design_par; also use gsub(fixed = TRUE) to avoid escaping parentheses as \(\)
an alternative way is a loop: for (i in c("design", "n", "event", "time", "bound", "power")) { # special case: Event -> Events names(ans)[names(ans) == i] <- if (i == "event") "Events" else { # capitalize the first letter sub("^(.)", "\\U\\1", i, perl = TRUE) } }
the pattern `x[match(y, names(x))]` could win an unmatched award... it should have been simply `x[y]`
…`filter()` can be simply indexing by name from `col_decimals`; then all `if` statments can be merged into a `for` loop
…ed to access the col_vars
…ame columns in `analyses`
…so been updated, so only need to add the `Null hypothesis` column
…tails`; just use `analyses` and `xy`
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 had to take several breaks while working on this PR, since it almost burned my brain several times. Sorry I can't make it bite-sized this time. Good luck to reviewers :)
} | ||
|
||
if ("event" %in% names(ans)) { | ||
ans <- ans %>% dplyr::rename(Events = event) |
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.
Is there a reason for renaming event
to Events
(plural) for fixed_design
but Event
(singular) for gs_design
?
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.
Please use "Events" for both fixed design and group sequential designs.
} | ||
} | ||
# If the method is WLR, change HR to wHR | ||
if (method == "wlr") xy <- replace_names(xy, c("~hr at bound" = "~whr at bound")) | ||
|
||
# If the method is COMBO, remove the column of "~HR at bound", and remove AHR from header |
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 comment is inconsistent with the code below---the code doesn't remove ~hr at bound
or ahr
. What should we do?
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 comment is correct, when it is maxcombo test, the "~HR at bound" and "AHR" do not exist.
@@ -138,7 +138,7 @@ | |||
\midrule\addlinespace[2.5pt] | |||
\multicolumn{6}{l}{Analysis: 1 Time: 36 N: 471.1 Event: 289 AHR: 0.68 Information fraction: 1\textsuperscript{\textit{3}}} \\[2.5pt] | |||
\midrule\addlinespace[2.5pt] | |||
Efficacy & 1.96 & 0.025 & 0.7940584 & 0.9 & 0.025 \\ | |||
Efficacy & 1.96 & 0.025 & 0.7941 & 0.9 & 0.025 \\ |
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 believe the changes in tests revealed a bug in the original code. That is, wHR at bound
should have been rounded to 4 decimal places by default but wasn't. My code fixed the bug, therefore I updated the test results here.
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.
No CI jobs are displayed for the PR because your most recent commit contained [ci skip]
. The most recent run was this one triggered by the previous push. I ran R CMD check
locally with the latest version, and I can confirm there is still only the single NOTE
about setNames
.
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.
Amazing work. Thanks @yihui!
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.
There are lots of work, thank you, @yihui !
…ith fixed design
…ith fixed design
Closes #282.