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

263 fixed design split into smaller export function #271

Merged

Conversation

LittleBeannie
Copy link
Collaborator

No description provided.

@LittleBeannie LittleBeannie self-assigned this Jul 17, 2023
@LittleBeannie LittleBeannie linked an issue Jul 17, 2023 that may be closed by this pull request
R/fixed_design_ahr.R Outdated Show resolved Hide resolved
R/fixed_design_fh.R Show resolved Hide resolved
R/fixed_design_mb.R Show resolved Hide resolved
R/fixed_design_rd.R Show resolved Hide resolved
R/fixed_design_ahr.R Outdated Show resolved Hide resolved
@@ -153,14 +151,14 @@ as_gt.fixed_design <- function(x, title = NULL, footnote = NULL, ...) {
"fh" = {
paste0(
"Power for Fleming-Harrington test ",
substr(x$design, 19, nchar(x$design)),
substr(x$Design, 19, nchar(x$Design)),

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why would we use upper cases now? I thought we should use lower cases.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for pointing it out! The x object is from gs_design/power_xxx() |> summary(), so all the column name is capitalized. Sentence case is used for summary output, and as_gt() is just for formate transformation (R table -> gt table). In this way, even if users don't use as_gt, they can use ... |> summary() |> gt() to get a sentence case table without any column renaming.


#' Fixed design sample size
#'
#' Computes fixed design sample size for Lachin-Foulkes method.
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As a user, I am confused what does LF method stands for.

A design needs to associate with a specific test. To me LF's paper is a general approach on piecewise enrollment and failure that can be applied to multiple tests.

If the function is related to log rank test with average hazard ration, do we really need this separate function? Or it is already covered by 'fixed_design_ahr'

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lf calls gsDesign functions. It is always hard to tell everything through a function name, so documentation (https://merck.github.io/gsDesign2/articles/usage-fixed-design.html) is important. If the documentation of fixed_design_xx is sufficient, then users can always go to the referenced paper for more details.

@LittleBeannie
Copy link
Collaborator Author

LittleBeannie commented Jul 18, 2023

I updated the documentation of fixed_design_xx in the lastest commit. Hope the method is more clear now.

@elong0527
Copy link
Collaborator

I updated the documentation of fixed_design_xx in the lastest commit. Hope the method is more clear now.

Thanks! I am fine to merge this PR given the good work you already have. Let's discuss function name in a meeting. Need some help to understand the details

@LittleBeannie
Copy link
Collaborator Author

Sure, let's merge the PR first but keep the issue open for Friday's discussion.

@LittleBeannie LittleBeannie merged commit 2e5bc43 into main Jul 18, 2023
8 checks passed
@LittleBeannie LittleBeannie deleted the 263-fixed_design-split-into-smaller-export-function branch July 18, 2023 15:50
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

fixed_design split into smaller export function
3 participants