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

Update pvalue_maxcombo() and move dplyr to Suggests #118

Merged
merged 1 commit into from
Oct 31, 2023

Conversation

jdblischak
Copy link
Collaborator

As part of #111, I updated pvalue_maxcombo(), which allowed me to move dplyr from Imports to Suggests

However, something to discuss, is I also updated the function signature. I found the mysterious dummy_var input variable to be confusing. It is only used when called by group_map(), and even then it's not obvious from the code how this is happening. Looking at the examples in ?group_map, they all use the syntax group_map(~ name_of_function(.x)), so I updated all the instances of group_map() to use this syntax. If dummy_var is preferred, I can revert this change.

From a speed perspective, the mean was mostly unchanged, but the median runtime was reduced almost 4x:

# before
library("microbenchmark")
library("simtrial")
x <- sim_fixed_n(
 n_sim = 1,
 timing_type = 5,
 rho_gamma = data.frame(
   rho = c(0, 0, 1),
   gamma = c(0, 1, 1)
 )
)
microbenchmark(pvalue_maxcombo(x), times = 1000)
## Unit: milliseconds
##                expr    min      lq     mean median     uq     max neval
##  pvalue_maxcombo(x) 2.9055 3.75245 5.722706 4.8051 5.6848 34.2989  1000


# after
microbenchmark(pvalue_maxcombo(x), times = 1000, unit = "ms")
## Unit: milliseconds
##                expr    min      lq     mean median      uq     max neval
##  pvalue_maxcombo(x) 0.6253 0.89085 5.068429 1.3186 4.59665 39.6136  1000

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.

dummy_var also looks a bit mysterious to me... I guess Keaven can tell if you revision makes sense and the intention. I checked the original repo history and this design was already there since the first version in 2019.

@nanxstats nanxstats merged commit 58751cd into Merck:main Oct 31, 2023
7 checks passed
@jdblischak jdblischak deleted the dt-pvalue_maxcombo branch October 31, 2023 13:59
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.

3 participants