-
Notifications
You must be signed in to change notification settings - Fork 4
42 add forest plot #198
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
42 add forest plot #198
Conversation
Code Coverage SummaryDiff against mainResults for commit: c04b6ca Minimum allowed coverage is ♻️ This comment has been updated with latest results |
|
@gravesti Hi Isaac, I tagged you as the reviewer for the initial forest plot function (Per our previous discussion, more discussions and updates are needed for the function). Please let me know if you have time to review or if you have some suggestions/thought. Greg also suggested that alternatively, I can put the function under design folder to bypass some of the lintr issue. Let me know what you think. Thank you! |
MikeJSeo
left a comment
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 for the nice code and nice plot! Works great!
I have reviewed it briefly.
Some comments I had:
- I think if bootstrap is run it should report bootstrap CI instead of default sandwich CI. So maybe check if bootstrap results are there.. and if there is replace it..
- Can you add a line in the vignettes: maybe just in unanchored_survival and anchored survival.Rmd a line that showcases this forest plot function? Maybe this you can do at the end..
- I put in a comment for case name change.. I think this makes better sense - to DISCUSS
- Let's put a dummy example instead of dontrun :)
- Need to Remove dplyr ggplot2 dependencies. Discuss about patchwork package dependency
…icplus into 42-add-forest-plot
|
There is a R CMD error after I removed dplyr, ggplot2 and patchwork from import. Is that ok? |
Suggestions for forest plot
Unit Test Performance Difference
Additional test case details
Results for commit 9f6876d ♻️ This comment has been updated with latest results. |
Unit Tests Summary 1 files 11 suites 12s ⏱️ Results for commit c04b6ca. |
Forest plot function added along with test files.