Skip to content
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

Merged
merged 43 commits into from
Sep 5, 2024
Merged

Clean up summary functions #461

merged 43 commits into from
Sep 5, 2024

Conversation

yihui
Copy link
Contributor

@yihui yihui commented Aug 30, 2024

Closes #282.

…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]`
@yihui yihui marked this pull request as ready for review September 5, 2024 05:04
Copy link
Contributor Author

@yihui yihui left a 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)
Copy link
Contributor Author

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?

Copy link
Collaborator

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
Copy link
Contributor Author

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?

Copy link
Collaborator

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 \\
Copy link
Contributor Author

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.

R/summary.R Show resolved Hide resolved
Copy link
Collaborator

@jdblischak jdblischak left a 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.

R/summary.R Show resolved Hide resolved
R/summary.R Outdated Show resolved Hide resolved
R/summary.R Show resolved Hide resolved
Copy link
Collaborator

@jdblischak jdblischak left a 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!

Copy link
Collaborator

@LittleBeannie LittleBeannie left a 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 !

@LittleBeannie LittleBeannie merged commit ea548ac into Merck:main Sep 5, 2024
7 checks passed
@yihui yihui deleted the summary-cleanup branch September 5, 2024 20:50
yihui added a commit to yihui/gsDesign2 that referenced this pull request Sep 6, 2024
yihui added a commit to yihui/gsDesign2 that referenced this pull request Sep 6, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Tidy up summary and as_gt functions
3 participants