Added estimate_KM.formula() method
#386
Conversation
| #' The function expects that the data has been filtered on the parameter (PARAM/PARAMCD) of interest. All NA values in the CNSR, AVAL and strata | ||
| #' argument are removed. | ||
| #' Alternatively, PARAM/PARAMCD can be used in the \code{strata} argument. \cr | ||
| #' |
There was a problem hiding this comment.
You started this paragraph with "The first is optimized ...." so I expect, as a reader, to see the second method explained as well briefly.
| #' Default is NULL. | ||
| #' @param AVAL Analysis value for Time-to-Event analysis. Default is "AVAL", as per CDISC ADaM guiding principles. | ||
| #' @param CNSR Censor for Time-to-Event analysis. Default is "CNSR", as per CDISC ADaM guiding principles. | ||
| #' @param data The name of the dataset. The dataset is expected to have |
There was a problem hiding this comment.
I want to keep the main focus on data standards to catch the attention of pharma so keep the reference to ADaM. You can then explain that we can also use non-ADaM eg
The name of the dataset for Time-to-Event analysis based on the Analysis Data Model (ADaM) principles ... When using a non-ADaM-like dataset, the formula method is recommended.
| @@ -240,7 +240,7 @@ testthat::test_that("T5.1 P-values are accurate when a filtered data frame is pi | |||
| ) %>% | |||
| {pchisq(.$chisq, length(.$n) - 1, lower.tail = FALSE)} %>% | |||
| visR:::.pvalformat() | |||
| testthat::expect_equal(survfit_p, survdiff_p) | |||
There was a problem hiding this comment.
Use prefixes everywhere.
| @@ -43,6 +43,9 @@ | |||
| #' T7.2 The function prefixes the function call with survival | |||
| #' T8. Piped datasets still return accurate results | |||
| #' T8.1 Piped datasets still return accurate results | |||
| #' T9. The user can specify formula argument | |||
| #' T9.1 The formula method returns the same results and the data method. | |||
| #' T9.2 The formula method triggers error messages. | |||
There was a problem hiding this comment.
Feel free to update the text to your preference
We can ask @bailliem to assist with the review if you don't have time at the moment.
I am not sure what you mean. A simpler approach is to add a formula argument and avoid generics/S3 methods (still my preference), but I think you preferred the S3 method when we discussed.
Yes, the next CRAN release is what was discussed. |
|
In @SHAESEN2 's review, I think he accurately points out that the S3 methods are overly complex given the added functionality, and I agree with him. We should opt for simply adding the I am going to close this PR for now, and we can discuss on Tuesday. |
What changes are proposed in this pull request?
estimate_KM()to a generic and added S3 methodsestimate_KM.data.frame()andestimate_KM.formula(). The data frame method is optimized for CDISC data, and the formula uses the standard survival syntax.The
estimate_KM()formula method is now marked as experimental and is not implemented for other functions at this time.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 #379
Checklist for PR reviewer
_pkgdown.ymlpkgdown::build_site(). Check the R console for errors, and review the rendered website.withr::with_envvar(new = c("NOT_CRAN" = "true"), covr::report()). Before you run, begin a fresh R session without any packages loaded.usethis::use_spell_check()runs with no spelling errors in documentationNEWS.mdbeen 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 (seeNEWS.mdfor examples).usethis::use_version(which = "dev")