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

Added check to get_pvalue() #389

Merged
merged 1 commit into from
May 29, 2022
Merged

Added check to get_pvalue() #389

merged 1 commit into from
May 29, 2022

Conversation

ddsjoberg
Copy link
Collaborator

@ddsjoberg ddsjoberg commented May 28, 2022

What changes are proposed in this pull request?

Did you include unit tests for the proposed change/bug fix (https://testthat.r-lib.org/)?
yes

If there is an GitHub issue associated with this pull request, please provide link.
closes #373

library(survival)
library(visR)

lung %>%
  survfit(Surv(time, status) ~ sex, data = .) %>%
  get_pvalue()
#> Error: There was an error calculating the p-values.
#> The 'survfit' object was not created with `visR::estimate_KM()`.
#> The the error will likely be resolved by re-estimating the 'survfit' object with visR.
#> Error in terms.formula(formula, "strata", data = data): object '.' not found

Created on 2022-05-28 by the reprex package (v2.0.1)


Checklist for PR reviewer

  • PR branch has pulled the most recent updates from main branch. Ensure the pull request branch and your local version match and both have the latest updates from the main branch.
  • If a new function was added, function should be included in _pkgdown.yml
  • If a bug was fixed, a unit test was added for the bug check
  • Run pkgdown::build_site(). Check the R console for errors, and review the rendered website.
  • Code coverage is suitable for any new functions/features. Review coverage with withr::with_envvar(new = c("NOT_CRAN" = "true"), covr::report()). Before you run, begin a fresh R session without any packages loaded.
  • R CMD Check runs without errors, warnings, and notes
  • usethis::use_spell_check() runs with no spelling errors in documentation
  • Has NEWS.md been updated with the changes from this pull request under the heading indicating the latest version. If there is an issue associated with the pull request, reference it in parentheses at the end update (see NEWS.md for examples).
  • Has the version number been incremented using usethis::use_version(which = "dev")
  • Approve Pull Request
  • Merge the PR. Please use "Squash and merge".

@ddsjoberg ddsjoberg requested a review from SHAESEN2 May 28, 2022 13:58
@ddsjoberg ddsjoberg changed the title Added check to check_pvalue() Added check to get_pvalue() May 28, 2022
@ddsjoberg ddsjoberg merged commit 920f6c8 into develop May 29, 2022
@ddsjoberg ddsjoberg deleted the is_visr_survfit branch May 29, 2022 12:19
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
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add check that 'survfit' object was created with estimate_KM()
2 participants