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

Minor proposal for readability of CI lines in the zipper plot #55

Merged
merged 2 commits into from
Dec 12, 2023

Conversation

lorenzo-guizzaro
Copy link
Contributor

Hi there,

while using this (great!) package, I have had some difficulties in seeing the CI lines of the zipper plot.

Thinking of changing their color to a more visible one, I have also thought to perhaps giving an option to make it clear whether there is optimal coverage or not. Please disregard if it does not make sense. In any case, thanks for making this package available!

@ellessenne
Copy link
Owner

Hi @lorenzo-guizzaro and thank you for this PR.

Could you paste some examples here to show the different looks under the ci_colors options, e.g., using {reprex}?

Thanks!

@lorenzo-guizzaro
Copy link
Contributor Author

library(ggplot2)
#> Warning: package 'ggplot2' was built under R version 4.3.2
library(devtools)
#> Warning: package 'devtools' was built under R version 4.3.2
#> Loading required package: usethis
#> Warning: package 'usethis' was built under R version 4.3.2
load_all("../rsimsum/")
#> ℹ Loading rsimsum
#> Warning: package 'testthat' was built under R version 4.3.2

data("relhaz", package = "rsimsum")
s1 <- simsum(
  data = relhaz, estvarname = "theta", se = "se", true = -0.50,
  methodvar = "model", by = c("n", "baseline"), x = TRUE
)
#> 'ref' method was not specified, Cox set as the reference

autoplot(s1, zoom = .3, type = "zip", ci_colors = 3)

autoplot(s1, zoom = .3, type = "zip", ci_colors = 2)

autoplot(s1, zoom = .3, type = "zip", ci_colors = 1)

Created on 2023-12-07 with reprex v2.0.2

@ellessenne
Copy link
Owner

Ok, looks good, thanks!

I have a few suggestion for the implementation:

  • Would rename the argument to zip_ci_colors to highlight that it only applies to zip plots;

  • Rather than having ci_colors = 1,2,3 I would have the user pass colour hex codes. So:

    1. If the user passes one string with a hex code, that is used as the yellow is now (equivalent to ci_colors = 1);
    2. If the user passes a vector with two hex codes, that is used for good/bad coverage scenarios (equivalent to ci_colors = 2);
    3. If the user passes a vector with three hex codes, that is used for too low/ok/too high coverage scenarios (equivalent to ci_colors = 3).

    Then, the default would be a single hex code (or color) zip_ci_colors = "yellow", thus leading to exactly the same plots (=reproducible code, no breaking changes), while giving flexibility to the user to customise the plot.

What do you think about the above?

@lorenzo-guizzaro
Copy link
Contributor Author

Thank you Alessandro, your ideas make much more sense and are best practice ;)

library(ggplot2)
#> Warning: package 'ggplot2' was built under R version 4.3.2
library(devtools)
#> Warning: package 'devtools' was built under R version 4.3.2
#> Loading required package: usethis
#> Warning: package 'usethis' was built under R version 4.3.2
load_all("../rsimsum/")
#> ℹ Loading rsimsum
#> Warning: package 'testthat' was built under R version 4.3.2

data("relhaz", package = "rsimsum")
s1 <- simsum(
  data = relhaz, estvarname = "theta", se = "se", true = -0.50,
  methodvar = "model", by = c("n", "baseline"), x = TRUE
)
#> 'ref' method was not specified, Cox set as the reference

autoplot(s1, zoom = .3, type = "zip", zip_ci_colors = c("green", "red", "blue"))

autoplot(s1, zoom = .3, type = "zip", zip_ci_colors = c("green", "red"))

autoplot(s1, zoom = .3, type = "zip", zip_ci_colors = c("yellow"))

autoplot(s1, zoom = .3, type = "zip")

Created on 2023-12-08 with reprex v2.0.2

@ellessenne
Copy link
Owner

Looks good – thanks 👍

Once the checks run I can merge this PR!

@ellessenne ellessenne merged commit 24c6d2a into ellessenne:master Dec 12, 2023
3 of 13 checks passed
ellessenne added a commit that referenced this pull request Dec 12, 2023
ellessenne added a commit that referenced this pull request Dec 12, 2023
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.

None yet

2 participants