Updates from review estimate_KM #405
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Updated estimate KM documentation to ensure to user knows that we allow non-CDISC. This was not clear.
ddsjoberg
approved these changes
Jun 12, 2022
There was a problem hiding this comment.
Changes look great. I made the following updates.
- The lifecycle package was listed twice in the DESCRIPTION file leading to a note in R CMD Checks. I removed one of the entries and ran
usethis::use_tidy_description()to put all DESCIRPTION items in the standard format, including alphabetizing package dependencies. This should help us see if a package has already been added when adding a new dependency. - There was an error message that was removed from
get_pvalue(). I re-added it, but in a way that maintains 100% code coverage. - Made slight modification to the text in the help file for estimate_KM
ddsjoberg
added a commit
that referenced
this pull request
Jun 22, 2022
* Wrote and executed watchdog * Add 'thevalidatoR' gh-action to generate a report on the release see https://github.com/marketplace/actions/r-package-validation-report * fixed formatting * Prototype * reformatting * major cleanup * added more sanity checks * Implemented compatibility for y-axis transformations and wrote some tests * More tests and now robust to fun=log * now robust against all function input? * Changed behaviour so that test aso works on CRAN and not only local * Added tests for incorrect arguments * Minor formatting changes * Dummy for the auto-deploy to shinyapps * Update deploy_to_shinyapps.yml * Update DESCRIPTION * Implemented Stevens suggestions * Removed parcats from imports, moved DT and gt to suggests, updated mans * align_plots: make more user friendly * add_risktabel: adjust ylabel according to increase in 4th column in grob of graph * Updated testing * - split visr testing file for each method - added more tests for visr survfit - updated visR to align with expectations - default method should not be a test. It should provide a default way of plotting objects * Camelcase to snakecase; minor formatting; initial vdiffr * forgot to save vdiffr changes * fixed bug since bools get translated to 0/1 which gets forwarded to base::plot and doesn't fail * Fixed formatting for CRAN compatibility * Fixed bug in testing * in progress * Update estimate_CUMINC.R * working on merge conflict * Wrote and executed watchdog * fixed formatting * reformatting * major cleanup * Implemented Stevens suggestions * working on merge conflict * Delete deploy_to_shinyapps.yml * package check update * in progress * Update check-standard.yaml * updates * Update check-standard.yaml * Update estimate_CUMINC.R * updates * Update DESCRIPTION * Update DESCRIPTION * Update DESCRIPTION * updates after review * in progress * updates * doc updates * first draft complete * init * Update test-estimate_CUMINC.R * misc updates * doc updates * Delete .Rprofile * Update DESCRIPTION * fix, now working on tests * Converting output to data.frame now * Extended watchdog to append END OF CODE section at the end of the file * addition of END OF CODE sections * Changed commit-date to ISO format (YYYY-MM-DDTHH:MM:SS) * updates * Updated testing * updates after review * updates * Update DESCRIPTION * rename * re-document * Update test-estimate_CUMINC.R * minor edit on test * Removed glue dependency * Re-build README.Rmd * Updated test 6.4 * Updated last-update-when-header * Attempt to fix riskmetric score in readme * Update README.Rmd * Attempt to fix riskmetric score in Readme * Update README.Rmd * Update README.Rmd * Removed unused snaps * Update rebuild-readme.yaml * Update rebuild-readme.yaml * Update README.Rmd * Update README.Rmd * Update rebuild-readme.yaml * Update rebuild-readme.yaml * Update README.Rmd * Update README.Rmd * Update test-coverage.yaml * Update rebuild-readme.yaml * Update README.Rmd * Update rebuild-readme.yaml * Update rebuild-readme.yaml * Update rebuild-readme.yaml * Update rebuild-readme.yaml * Update rebuild-readme.yaml * Re-build README.Rmd * Update rebuild-readme.yaml * Update README.Rmd * Update test-coverage.yaml * Fixed small error in numbering of tests * Adding cumulative events and censored obs to survfit `*_risktable()` fns * Updated to align legend labels with strata specified * Re-build README.Rmd * remove tidyselect dependency * removing magrittr dep * don't reexport %||% * Regenerated man file for estimate_cuminc after rename * Update get_risktable.R * renamed cumulative.* to cum.* * Updated testing for PR310 * Included survfit object to tidyme output as attribute Included broom colname nomenclature in tidyme output * Re-build README.Rmd * update naming in visR as well * fixed bug * Updated documentation and added CRAN check \value tag exceptions to watchdog * Fixed typo in test, now package check is back at 0/0/0 * Fixed threeway test * Made new test less ambigous; minor formatting * Readress issue * adressed comments * Re-build README.Rmd * risktable updates * Re-build README.Rmd * rlang prefix updates * Re-build README.Rmd * adding images of rendered tableone() tables in help file * Re-build README.Rmd * Update README.Rmd * Re-build README.Rmd * Specified riskmetric version to avoid breaking 0.1.2 * Reverted useless change * Re-build README.Rmd * Integrate learnR vignette and shinyapps.io autodeploy (#339) * Update DESCRIPTION * Created deploy_to_shinyapps.yml * Changed branch to be deployed to develop * Added gt to suggests * Re-build README.Rmd * Added test for external data used in vignettes * Re-build README.Rmd Co-authored-by: GitHub Actions <actions@github.com> * Added link target of shinyapps autodeploy * Adding R CMD Checks for older versions of R (#324) * workflow updates * Re-build README.Rmd * plot() fix * Fix broken code and updated vignettes * Update test-watchdog_testfiles.R * Re-build README.Rmd Co-authored-by: GitHub Actions <actions@github.com> Co-authored-by: shaesen2 <shaesen2@its.jnj.com> Co-authored-by: Tim Treis <tim.treis@outlook.de> * Build `README.Rmd` on main branch instead of all PRs (#342) * build on main instead of all PRs * Update rebuild-readme.yaml Co-authored-by: Tim Treis <tim.treis@outlook.de> * Changed R version to be shinyapps.io compatible * Reverted "build README on main" because bot can't push to write-protected branch * Updates to website (#345) * first draft of updated website * Update _pkgdown.yml * Updated NEWS with changelog (#349) * Updated NEWS with changelog Quick fixes for readme to remove contributor stats and cover stats (can be accessed through the badge) * fixed typo * fixed typo * Re-build README.Rmd * update ReadMe and trigger readme flow from inside pushed branch * Re-build README.Rmd * Implemented review and adjusted readme.RMD * Re-build README.Rmd * fixed readme * fixed readme * dunno * Re-build README.Rmd * Updated contributors * Re-build README.Rmd * Adressed review comments - Badges side-by-side - Updated NEWS Co-authored-by: shaesen2 <shaesen2@its.jnj.com> Co-authored-by: GitHub Actions <actions@github.com> * Create contributing (#326) * Create contributing * Re-build README.Rmd * Updated contributing * Re-build README.Rmd Co-authored-by: GitHub Actions <actions@github.com> Co-authored-by: Mark Baillie <bailliem@gmail.com> Co-authored-by: shaesen2 <shaesen2@its.jnj.com> * Updated the strata levels and label in `visr()` (#348) * updating default strata labels/headers * label updates * Re-build README.Rmd * Update DESCRIPTION * Update add_CNSR.R * Re-build README.Rmd * updates after review * doc update * Update test-estimate_KM.R * Re-build README.Rmd * Update estimate_cuminc.R * updates after review * doc updates * Re-build README.Rmd * review updates * more review updates * estimate_KM Updated tests to be more clear Updated comments to make more clear Co-authored-by: GitHub Actions <actions@github.com> Co-authored-by: shaesen2 <shaesen2@its.jnj.com> * NEWS entry update (#351) * updated news and increment version number * Update NEWS.md * Re-build README.Rmd Co-authored-by: GitHub Actions <actions@github.com> * Create PR template (#330) * Create PULL_ * Re-build README.Rmd * Update PR template * Updated based on review * Re-build README.Rmd * Re-build README.Rmd Co-authored-by: GitHub Actions <actions@github.com> Co-authored-by: shaesen2 <shaesen2@its.jnj.com> Co-authored-by: Tim Treis <tim.treis@outlook.de> Co-authored-by: Daniel Sjoberg <danield.sjoberg@gmail.com> * Adding vignette example for estimate_cuminc() (#355) * adding vignette example for estimate_cuminc() * Re-build README.Rmd * Updated vignettes * Re-build README.Rmd Co-authored-by: GitHub Actions <actions@github.com> Co-authored-by: shaesen2 <shaesen2@its.jnj.com> * Added update_documentation to quickly update everything and generate … (#356) * Added update_documentation to quickly update everything and generate documentation/visR.pdf Updated S3 method roxygen * Removed update_documentation implemented review comments * Increment version number to 0.2.0.9001 Co-authored-by: shaesen2 <shaesen2@its.jnj.com> Co-authored-by: Daniel Sjoberg <danield.sjoberg@gmail.com> * Updates to `class()` calls (#359) * class updates * Update test-watchdog_testfiles.R * Re-build README.Rmd * Further updates to class vs inherits * Delete visR.pdf Co-authored-by: GitHub Actions <actions@github.com> Co-authored-by: shaesen2 <shaesen2@its.jnj.com> * Review of Joanna testing (#361) * Review of Joanna testing estimate_cuminc -------------- - Initialize data argument - focus on arguments that are required for ADaM eg confint can be defined through ... Tidycmprisk should deal with confint variations - add more informative messages - Do we want to add an overall strata? We can put Overall instead of 1 inside the formula and then add a column strat99= Overall - %||% is not defined. When strata is NULL then the formula becomes ...... ~ NULL in the final object without any error => used ifelse statement testing ------- - we need to focus on what the wrapper is doing, not on what the tidycmprisk function -- test arguments -- test excecuting surrounding the main function -- do not test how the main function works - downstream processing of this object needs to be tested in the respective test files, not in the estimate function - we expect tidycmprisk to do the heavy lifting and testing on the final object. We only verify wether the wrapper does not change the object. If it do * Re-build README.Rmd * review updates * Update _pkgdown.yml * adding `visR::` prefix in examples * review updates from call * Re-build README.Rmd * update for conf.int * Re-build README.Rmd * redocument Co-authored-by: shaesen2 <shaesen2@its.jnj.com> Co-authored-by: GitHub Actions <actions@github.com> Co-authored-by: Daniel Sjoberg <danield.sjoberg@gmail.com> * Test/add annotation cuminc updates (#364) * update T1 to include cuminc test * update T2 to include cuminc tests * update T3 to include cuminc * update T5 to include cuminc * update T6 * Fix T3.2 extracted_lbl * spell check updates (#372) * spell check updates * Re-build README.Rmd Co-authored-by: GitHub Actions <actions@github.com> * Update README.Rmd by tableone (#369) * Update README.Rmd by tableone * Re-build README.Rmd * installing and rendering readme * Re-build README.Rmd * updates * Update README.md * Update NEWS.md * Increment version number to 0.2.0.9002 * Re-build README.Rmd Co-authored-by: GitHub Actions <actions@github.com> Co-authored-by: Daniel Sjoberg <danield.sjoberg@gmail.com> * Added table.height to add_risktable (#353) * Added table.height to add_risktable * Re-build README.Rmd * unit tests were added * Complete tests + rename var + doc * Added small fixes * Delete test-estimate_CUMINC.R * fix doc reference * Re-build README.Rmd * Update rowgutter default value Co-authored-by: GitHub Actions <actions@github.com> Co-authored-by: shaesen2 <shaesen2@its.jnj.com> Co-authored-by: Daniel Sjoberg <danield.sjoberg@gmail.com> * Update readme tableone display (#376) * update readme tableone display * Re-build README.Rmd * Re-build README.Rmd Co-authored-by: GitHub Actions <actions@github.com> * Add na.rm option to remove the strata variable in rows (#368) * Add na.rm option to remove the strata variable in rows * update unit tests * Re-build README.Rmd * remove na.rm and applied strata filteration * Re-build README.Rmd * Update test-get_tableone.R * Update WORDLIST * increment version number * Re-build README.Rmd Co-authored-by: Daniel Sjoberg <danield.sjoberg@gmail.com> Co-authored-by: GitHub Actions <actions@github.com> * Removing warning in `visr()` when PARAMCD not present (#380) * Update visr.R * Update test-visr_survfit.R * Re-build README.Rmd * documentation update * doc updates * increment version number * Re-build README.Rmd Co-authored-by: GitHub Actions <actions@github.com> * Replaced `survfit` call with quo (#366) * replace survfit call with quo * Re-build README.Rmd * Update test-get_pvalue.R * Re-build README.Rmd * Update get_COX_HR.R * Re-build README.Rmd * Update test-watchdog_testfiles.R * Re-build README.Rmd * small adjustments to improve readability * Re-build README.Rmd * updates from review * Re-build README.Rmd * Re-build README.Rmd * doc update * Re-build README.Rmd * Re-build README.Rmd * Added prefix for testthat. Updated documentation for estimate_KM. Removed warning param/paramcd. * Re-build README.Rmd * review updates * Re-build README.Rmd * increment version number Co-authored-by: GitHub Actions <actions@github.com> Co-authored-by: shaesen2 <shaesen2@its.jnj.com> * Adding unit tests with cumulative inc. (#387) * adding unit tests * Update get_risktable.Rd * visr survfit check (#389) * WIP: start documenting all functions in render.R * WIP: add roygen skeleton * WIP: remove export for all summarize functions * WIP: remove export for align_plots function * WIP: remove export from get_tableone function * WIP: remove export from get_summary function * WIP: remove export from lhs/pipe function * WIP: remove export for get_risktable and quantilte funcs * Remove lifecycle badge and text. Move remaining badges under installation. * Re-build README.Rmd * add lifecycle to description and experimental badge to render function * document royxgen headings for all functions in render.R. compile documentation. * export what is needed * export what is needed * Re-build README.Rmd * move badge location to under first section - * Re-build README.Rmd * adding lifecycle to wordlist * bump package increment * export * get_summary * align_plots * package bump * tidy up methods of the same s3 class and remove duplication in docs * Re-build README.Rmd * roll back some of the export deletions. trigging test failures and also required for vignettes, examples. * get_* * utils pipe * utilts table * Re-build README.Rmd * WIP: connect the s3 methods for summarize through document. * Tidy up tags * clean up typo and build docs * bump package add experimental tag to get_tableone * Adding agument `estimate_KM(formula=)` (#390) * adding formula argument * adding lifecycle badges * doc update * Re-build README.Rmd * Update test-estimate_KM.R * Re-build README.Rmd * cleaning up before requesting review * review comment updates * typo * Update estimate_KM.R * doc update * Re-build README.Rmd * updates * increment version number Co-authored-by: GitHub Actions <actions@github.com> * Updated to GH Actions V2 * Re-build README.Rmd * Add instruction to check if the readme has been rendered manually. (#404) * Add instruction to check if the readme has been rendered manually. * build readme * Updates from review estimate_KM (#405) * Updated estimate KM testing Updated estimate KM documentation to ensure to user knows that we allow non-CDISC. This was not clear. * Ran `usethis::use_tidy_description()` to put DESCRIPTION entries in standard order * docs * re-adding the error message that was removed. * docs Co-authored-by: shaesen2 <shaesen2@its.jnj.com> Co-authored-by: Daniel Sjoberg <danield.sjoberg@gmail.com> * Documentation updates (#409) * doc updates * doc updates * Adding `Surv_CNSR()` function (#397) * Create Surv_CDISC.R * updates * doc update * Re-build README.Rmd * Update Surv_CDISC.R * Re-build README.Rmd * Update Surv_CDISC.R * Re-build README.Rmd * doc updates * Re-build README.Rmd * Update NAMESPACE * merge conflict fix * Re-build README.Rmd * updates * Re-build README.Rmd * Update test-Surv_CDISC.R * Re-build README.Rmd * readme * doc updates from review * doc updates * Update test-Surv_CDISC.R * Update testing: - consistent with CDISC philosophy - check the actual requirement - remove req T2.5 as this is not a requirement of our function, but something tested in the backend of survival * removed requirement 2.3 in TOC * change function name * Update testing Included CNSR info in estimate_KM to ensure user will restrict to 0/1 * re-documenting Co-authored-by: GitHub Actions <actions@github.com> Co-authored-by: Mark Baillie <bailliem@gmail.com> Co-authored-by: shaesen2 <shaesen2@its.jnj.com> * Update test-Surv_CNSR.R (#410) * fix issues associated with this function * issue fix * Add @nord to internal functions. (#412) Add questioning to functions under development / review * Added test for case when not all colours are defined. * Tiny bugfix, two more tests * Update testing of theming * Updated code/testing for coverage * Include legend title testing * Knit readme Move lifecycle badges to under description Add questioning badge to tabelone * add update to news,md * styler + spell check (#414) * Update pull_request_template.md Co-authored-by: Tim Treis <tim.treis@outlook.de> Co-authored-by: James Black <james@epijim.uk> Co-authored-by: shaesen2 <haesendonckx.steven@gmail.com> Co-authored-by: Tim Treis <tim.treis@stud.uni-heidelberg.de> Co-authored-by: shaesen2 <shaesen2@its.jnj.com> Co-authored-by: GitHub Actions <actions@github.com> Co-authored-by: Steven Haesendonckx <47894155+SHAESEN2@users.noreply.github.com> Co-authored-by: Mark Baillie <bailliem@gmail.com> Co-authored-by: Joana M. Barros <94060211+joanacmbarros@users.noreply.github.com> Co-authored-by: ardeeshany <ardeeshany@gmail.com>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Updated estimate KM documentation to ensure to user knows that we allow non-CDISC. This was not clear.
Updated estimate KM testing