-
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
Implement sim_gs_n()
#195
Implement sim_gs_n()
#195
Conversation
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? |
I like that approach |
ans <- data |> | ||
counting_process(arm = "experimental") |> | ||
mb_weight(delay = weight$delay, w_max = weight$w_max) |> | ||
dplyr::summarize( |
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 can also convert this to {data.table} since we have moved {dplyr} to Suggests
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.
That would be awesome...
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.
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 beseq_len(n)
. -
"zzz" %in% class(x)
should beinherits(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()
andname_weight()
functions coexist wherename
=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.
Closes #194