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

Difference between if statement and error message in ggsurv_m #222

Closed
johan-ejstrud opened this issue Dec 29, 2016 · 3 comments
Closed

Difference between if statement and error message in ggsurv_m #222

johan-ejstrud opened this issue Dec 29, 2016 · 3 comments

Comments

@johan-ejstrud
Copy link

When using confidence intervals in a plot with multiple strata in ggsurv(), the error message "Either surv.col or lty.est should be of length 1 in order to plot 95% CI with multiple strata" can be encountered. However, the if statement and the error message does not agree:

if (length(surv.col) > 1 || length(lty.est) > 1) {
   stop("Either surv.col or lty.est should be of length 1 in order to plot 95% CI with multiple strata")

I think that the error message is correct, while the if statement is not. My guess is that the code should be changed to:

if (length(surv.col) != 1 & length(lty.est) != 1) {
   stop("Either surv.col or lty.est should be of length 1 in order to plot 95% CI with multiple strata")
@schloerke
Copy link
Member

@EdwinTh

Do you think this error message is needed anymore?

stop('Either surv.col or lty.est should be of length 1 in order to plot 95% CI with multiple strata')

I think surv.col length check is covered here:

pl <- if(surv.col[1] != 'gg.def'){

and lty.est length check is covered here:
lineScaleValues <- if (length(lty.est) == 1) {

If so, we might as well remove the stop check.

@EdwinTh
Copy link

EdwinTh commented Jan 5, 2017

I cannot figure out why I introduced the stop check in the first place. Although specifying both multiple surv.col and multiple lty.est gives a bit of a circus, technically there is nothing wrong with it.

The checks you mention, Barret, do not check for the combination of the two parameters, so in that sense the check is not redundant. However I still opt for removing it, because it is for the user to decide if both should be specified.

@schloerke
Copy link
Member

removed error check. submitting to cran

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

No branches or pull requests

3 participants