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

Convert mb_weight() to data.table #115

Merged
merged 2 commits into from
Oct 30, 2023
Merged

Conversation

jdblischak
Copy link
Collaborator

@jdblischak jdblischak commented Oct 27, 2023

I chose mb_weight() as the next function to convert since it would allow us to move tidyr from Imports to Suggests.

The speed increase was ~2x, but that wasn't the primary motivation in this case.

library("microbenchmark")

# before
microbenchmark(mb_weight(x, delay = 6, w_max = Inf), times = 1000)
## Unit: milliseconds
##                                  expr     min      lq    mean   median       uq      max neval
##  mb_weight(x, delay = 6, w_max = Inf) 11.0001 11.7249 13.5589 12.19605 13.46255 186.3355  1000

# after
microbenchmark(mb_weight(x, delay = 6, w_max = Inf), times = 1000)
## Unit: milliseconds
##                                  expr    min      lq     mean median      uq      max neval
##  mb_weight(x, delay = 6, w_max = Inf) 3.8054 4.19625 5.571988 4.5562 5.48045 168.4506  1000

Also note that I had to temporarily disable skip_if_offline() because it uses r-project.org, which has been down all morning

xref: #111

@jdblischak
Copy link
Collaborator Author

I also made a few updates to the vignette that explains mb_weight(). I think the section Freidlin and Korn strong null hypothesis example needs some more attention. For example, I don't think this line below about the failure rate is correct:

The control group has an exponential failure rate with a median of 15 months.

@jdblischak
Copy link
Collaborator Author

Also, it would be ideal to add a test case that involves a stratum that gets dropped by the filter:

simtrial/R/mb_weight.R

Lines 153 to 156 in 0f06d96

# Get back stratum with no records before delay ends
right_join(tbl_all_stratum, by = "stratum") %>%
# `max_weight` is 1 when there are no records before delay ends
mutate(max_weight = replace_na(max_weight, 1)) %>%

@nanxstats
Copy link
Collaborator

This looks good to me - you will want to check in with the original author of vignettes/modestWLRTVignette.Rmd about the minor changes.

@nanxstats nanxstats merged commit 7daf1e3 into Merck:main Oct 30, 2023
7 checks passed
@jdblischak jdblischak deleted the dt-mb_weight branch October 30, 2023 13:47
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.

2 participants