-
-
Notifications
You must be signed in to change notification settings - Fork 21
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
401 add rate01 tlg@main #739
Conversation
Code Coverage Summary
Diff against main
Results for commit: 181aa89 Minimum allowed coverage is ♻️ This comment has been updated with latest results |
R/summarize_glm_count.R
Outdated
#' library(scda) | ||
#' library(dplyr) | ||
#' anl <- synthetic_cdisc_dataset("latest", "adtte") %>% | ||
#' filter(PARAMCD == "TNE") | ||
#' anl$AVAL_f <- as.factor(anl$AVAL) |
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 don't think this part is needed, as the below example is not ran
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 think it is needed for the last example which should be on the same doc file. Still, I would not repeat this there
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.
the last example has its own already
.null_ref_cells = FALSE | ||
) | ||
|
||
#' @describeIn summarize_glm_count Layout creating function which can be be used for creating |
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 think we can move from line 411 to the end of function summarize_glm_count to the top of this script, so the documentation is clearer. As all the other parts are internal, and examples won't need to be run.
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.
It makes sense but we can always put comment titles to describe the "internal" steps. Not sure. Maybe you are right and having only "runnable" docs is better
library(scda) | ||
library(dplyr) | ||
anl <- synthetic_cdisc_dataset("latest", "adtte") %>% | ||
filter(PARAMCD == "TNE") | ||
anl$AVAL_f <- as.factor(anl$AVAL) |
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 think this part of data loading can be external to the testing functions. so the testing data can be reused, and will be more efficient.
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.
actually the libraries should be in setup_libraries and the dataset should be taken from the ones in setup_data
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.
Excellent! That is the proper way to do it now! Thanks @Melkiades
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.
Thanks @ayogasekaram ! it looks really good. I made some samll change requests. thanks!
Great job Abi!! :) I did not review it in detail, as Joe is on it, but maybe we could aim at a bit higher coverage (>90%) |
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.
changes lgtm! Thanks @ayogasekaram
added unit tests for summarize_glm_count
adding rate01 to tlg MR found here: https://code.roche.com/nest/docs/tlg-catalog/devel/-/merge_requests/162
closes #538