Fix bug in ard_survival_survfit() when "=" is in strata variable labels#253
Fix bug in ard_survival_survfit() when "=" is in strata variable labels#253
ard_survival_survfit() when "=" is in strata variable labels#253Conversation
edelarua
changed the title
ard_survival_survfit() when "=" is in strata variable labels
Unit Tests Summary 1 files 162 suites 1m 11s ⏱️ Results for commit 1d377c9. ♻️ This comment has been updated with latest results. |
Unit Test Performance Difference
Additional test case details
Results for commit 816377c ♻️ This comment has been updated with latest results. |
R/ard_survival_survfit.R
Outdated
| if ("strata" %in% names(ret)) { | ||
| ret <- ret %>% | ||
| tidyr::separate_wider_delim("strata", "=", names = c("group1", "group1_level")) | ||
| tidyr::separate_wider_delim("strata", "=", names = c("group1", "group1_level"), too_many = "merge") |
There was a problem hiding this comment.
It seems this code is only run when there is a single stratifying variable, right? Do we know the variable name at this point? Would it be more robust to split this using the variable name, so it's straightforward to split correctly no matter what?
There was a problem hiding this comment.
At this point in the function the untidied model is no longer carried over to extract the strata variable name from, and the broom::tidy() results that we have don't include the variable name separately, so I think this is the easiest way to split. Alternatively, we could extract the strata variable name and add it as another argument to the formatting function.
What do you think?
There was a problem hiding this comment.
I think I am a little confused. The internals have a function called extract_multi_strata() that processes the stratifying variable names. But there is a condition, that it only parses when there is strictly greater than one stratifying variable. The consequence of this is that when our model has two variables, there was no issue with parsing a level that included an "=" sign. But this section of code is skipped when there is one variable. Can we just change that condition if (length(x_terms) > 0L) ?
extract_multi_strata <- function(x, df_stat) {
x_terms <- attr(stats::terms(stats::as.formula(x$call$formula)), "term.labels")
x_terms <- gsub(".*\\(", "", gsub("\\)", "", x_terms))
if (length(x_terms) > 1) {There was a problem hiding this comment.
Maybe I am missing something?
There was a problem hiding this comment.
Sorry, you're correct. I was getting confused by the order that the functions were applied, but I have updated the code to process all strata in the same place now.
ddsjoberg
left a comment
ddsjoberg
left a comment
There was a problem hiding this comment.
Beautiful! Thank you for the update!!!
2 participants
What changes are proposed in this pull request?
ard_survival_survfit()causing an error when "=" character is present in stratification variable level labels. (ard_survival_survfit fails when stratification variable level includes a "=" #252)Closes #252
Pre-review Checklist (if item does not apply, mark is as complete)
usethis::pr_merge_main()ard_*()function was added, it passes the ARD structural checks fromcards::check_ard_structure().ard_*()function was added,set_cli_abort_call()has been set.ard_*()function was added and it depends on another package (such as,broom),is_pkg_installed("broom")has been set in the function call and the following added to the roxygen comments:@examplesIf do.call(asNamespace("cardx")$is_pkg_installed, list(pkg = "broom""))devtools::test_coverage()Reviewer Checklist (if item does not apply, mark is as complete)
devtools::test_coverage()When the branch is ready to be merged:
NEWS.mdwith the changes from this pull request under the heading "# cardx (development version)". If there is an issue associated with the pull request, reference it in parentheses at the end update (seeNEWS.mdfor examples).