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

Implement sim_gs_n() #195

Merged

Conversation

LittleBeannie
Copy link
Collaborator

@LittleBeannie LittleBeannie commented Feb 9, 2024

Closes #194

@LittleBeannie LittleBeannie added the development New feature or request label Feb 9, 2024
@LittleBeannie LittleBeannie self-assigned this Feb 9, 2024
@LittleBeannie LittleBeannie linked an issue Feb 9, 2024 that may be closed by this pull request
@nanxstats nanxstats marked this pull request as draft February 9, 2024 20:25
@nanxstats nanxstats changed the title sim_gs_n development Implement sim_gs_n() Feb 9, 2024
@LittleBeannie
Copy link
Collaborator Author

Hello @nanxstats and @jdblischak , this pull request (PR) is now ready for you to proceed with in the functional factory. Please keep in mind that this PR primarily focuses on cuttings and testings, and the parallel simulations have NOT been implemented yet (we can address them in a separate PR). Given the numerous updates in this PR, would you prefer to merge it first and then create a PR specifically for the functional factory?

@jdblischak
Copy link
Collaborator

Given the numerous updates in this PR, would you prefer to merge it first and then create a PR specifically for the functional factory?

I like that approach

ans <- data |>
counting_process(arm = "experimental") |>
mb_weight(delay = weight$delay, w_max = weight$w_max) |>
dplyr::summarize(
Copy link
Collaborator

Choose a reason for hiding this comment

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

I can also convert this to {data.table} since we have moved {dplyr} to Suggests

Copy link
Collaborator

Choose a reason for hiding this comment

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

That would be awesome...

@LittleBeannie LittleBeannie marked this pull request as ready for review February 12, 2024 17:43
Copy link
Collaborator

@nanxstats nanxstats left a comment

Choose a reason for hiding this comment

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

Great, let's merge this first to move it forward. I believe @jdblischak can show us the ideal functional interface and a performant data.table backend. 🥇

Besides the important things above... it would be great if you could also include a few minor improvements on code style:

  • Avoid adding rlang back to Imports, considering we have just made an effort to remove it from there.

  • 1:n should be seq_len(n).

  • "zzz" %in% class(x) should be inherits(x, "zzz").

  • Use year 2024 in the copyright header.

  • Boilerplate code like this:

    ans <- list(rho = rho, gamma = gamma)
    class(ans) <- c(class(ans), "fh", "wlr")
    return(ans)

    can be simplified to a one-liner:

    structure(list(rho = rho, gamma = gamma), class = c("list", "fh", "wlr"))

    See r-base-shortcuts.

  • It's a bit unclear why we should have name() and name_weight() functions coexist where name = fh, mb, early_zero,... Perhaps this duplication will become obsolete after the API refactor - having a single source of truth and not having to read documentation to use functions correctly would be great.

@nanxstats nanxstats merged commit 52c26bd into main Feb 13, 2024
7 checks passed
@nanxstats nanxstats deleted the 194-develop-sim_gs_n-for-group-sequential-design-simulations branch February 13, 2024 00:47
jdblischak added a commit to jdblischak/simtrial that referenced this pull request Feb 22, 2024
jdblischak added a commit to jdblischak/simtrial that referenced this pull request Feb 23, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
development New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Develop sim_gs_n for group sequential design simulations
3 participants