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

Pull Request by Zilin Huang regarding Issue #216 #226

Open
wants to merge 11 commits into
base: main
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
10 changes: 9 additions & 1 deletion R/ggcuminc.R
Original file line number Diff line number Diff line change
Expand Up @@ -55,6 +55,14 @@ ggcuminc <- function(x, outcome = NULL,
)
}

# Generate an error if the input is a `survfit.coxphms` regression model: ----
if (inherits(x, "survfitcoxms")) {
cli_abort(
"Argument {.arg x} does not support {.cls survfit.coxphms} object.
Please fit a multi-state model with another project, such as {.fn coxph}."
Comment on lines +61 to +62
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is great, thank you for adding. Let's keep the message simple with "Argument {.arg x} does not support {.cls survfit.coxphms} object.".

Can you also add a unit test to test appropriate error messaging with expect_error()?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @ddsjoberg, I'll work with the unit test later (on Issue #222 PR)

)
}

# prep data to be passed to ggplot() -----------------------------------------
if (inherits(x, "tidycuminc"))
df <- tidy_cuminc(x = x)
Expand All @@ -69,7 +77,7 @@ ggcuminc <- function(x, outcome = NULL,
cli_inform("Plotting outcome {.val {outcome}}.")
}
if (any(!outcome %in% unique(df$outcome))) {
cli_abort("Argument {.code outcome} must be one or more of {.val {unique(df$outcome)}}")
cli_abort("Argument {.arg x} does not support {.cls survfit.coxphms} object.")
}
df <- dplyr::filter(df, .data$outcome %in% .env$outcome)

Expand Down
17 changes: 16 additions & 1 deletion R/scale_ggsurvfit.R
Original file line number Diff line number Diff line change
Expand Up @@ -34,8 +34,23 @@
#' # specify additional scales
#' ggsurvfit +
#' scale_ggsurvfit(x_scales = list(breaks = seq(0, 8, by = 2)))
#'
#' @details
#' Specical case: in the risk table, large numbers (with more than 4 digits) may not
#' be shown completely, with some digits truncated outside the plot region.
#' To remedy this, consider the following example code for a simulated large-size survival dataset:
#' Here, adjust the expand size in `scale_ggsurvfit(x_scales = list(expand = c(0.05, 0)))` (for example)
#' can modify the position of numbers in the risk table
#' and make them all fit in the plot region. The scale of the `expand` argument differs by cases.
#'
#' df_colon_large <- df_colon[sample(1:nrow(df_colon), size = 15*nrow(df_colon), replace = TRUE), ]
#' ggsurvfit <-
#' survfit2(Surv(time, status) ~ surg, data = df_colon_large) %>%
#' ggsurvfit() + add_risktable() +
#' scale_ggsurvfit()
Comment on lines +39 to +50
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks great, thank you!

upon looking at it, perhaps we don't need the full example, and just keep this part: scale_ggsurvfit(x_scales = list(expand = c(0.05, 0)))

#' @inherit ggsurvfit seealso
scale_ggsurvfit <- function(x_scales = list(), y_scales = list()){
scale_ggsurvfit <-
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you put this back on one line?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have already done this. It'll be ready to merge.

function(x_scales = list(), y_scales = list()){
scale_ggsurvfit_empty_list <- list()
structure(scale_ggsurvfit_empty_list, x_scales = x_scales, y_scales = y_scales, class = "scale_ggsurvfit")
}
Expand Down
2 changes: 1 addition & 1 deletion R/utils-add_risktable.R
Original file line number Diff line number Diff line change
Expand Up @@ -18,7 +18,7 @@
}

# get data to place in risktables --------------------------------------------
times <- times %||% plot_build$layout$panel_params[[1]]$x.sec$breaks
times <- times %||% plot_build$layout$panel_params[[1]]$x$breaks
df_times <-
.prepare_data_for_risk_tables(data = x$data, times = times, combine_groups = combine_groups)

Expand Down
15 changes: 15 additions & 0 deletions man/scale_ggsurvfit.Rd

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

47 changes: 8 additions & 39 deletions man/stepribbon.Rd

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.