pharmaverse / ggsurvfit

http://www.danieldsjoberg.com/ggsurvfit/
Other
70 stars 19 forks source link

Plot arithmetic #115

Closed ddsjoberg closed 11 months ago

ddsjoberg commented 1 year ago

What changes are proposed in this pull request?

Caveats:

image

If there is an GitHub issue associated with this pull request, please provide link. closes #96


Reviewer Checklist (if item does not apply, mark is as complete)

When the branch is ready to be merged into master:

michaelcurry1123 commented 1 year ago

If you try to use the operators with 3 plots (with any operators) it throws an error.

p1 <- survfit2(Surv(time, status) ~ sex, data = df_lung) %>%
  ggsurvfit() +
  add_risktable()

p2 <- survfit2(Surv(time, status) ~ sex, data = df_lung) %>%
  ggsurvfit() +
  add_risktable()

p3 <- survfit2(Surv(time, status) ~ sex, data = df_lung) %>%
  ggsurvfit() +
  add_risktable()

p1 | p2 | p3

Error in p1 | p2 | p3 : operations are possible only for numeric, logical or complex types In addition: Warning message: Incompatible methods ("|.ggplot", "|.ggsurvfit") for "|"

Also this doesn't work: (p1 | p2) / p3

michaelcurry1123 commented 1 year ago

The horizontal stack makes numbers in risk tables overlap.

p1 <- survfit2(Surv(time, status) ~ sex, data = df_lung) %>% ggsurvfit() + add_risktable()

p2 <- survfit2(Surv(time, status) ~ sex, data = df_lung) %>% ggsurvfit() + add_risktable()

p1/p2

image

ddsjoberg commented 1 year ago

Thanks for taking a look @michaelcurry1123 !

Regarding the first point: The issue is that after p1 | p2 is run, the result is no longer a ggsurvfit object (it's a regular patched together plot at that point. So when that result is then dispatched to . | p3, we're no longer using the |.ggsurvfit method and it's being dispatched via the patchwork method and it doesn't know how to handle the ggsurvfit plot.

Potential solutions:

  1. Making the documentation more clear about this issue.
  2. We could attempt to export conflicting method for ggplot class (it would mask the patchwork method or vice versa) so we can explicitly handle the second argument when it is class ggsurvfit.

I think for now, we'll need to opt for option one...I don't like the idea of exporting a conflicting method with patchwork AND I think it will be tricky to export this method while also trying to utilize the patchwork method in the background: there will be conflict.

Regarding the second point: That is just the default spacing. If you make the figure taller, the plots and risk tables will display appropriately

michaelcurry1123 commented 1 year ago

Ok that makes sense and if someone wants that customization you can do it like so:

library(gridExtra)

grid.arrange(p1,p2,p3,
             layout_matrix = 
              matrix(
                c(1,1,1,2,2,2,
                1,1,1,2,2,2,
                1,1,1,2,2,2,
                1,1,1,2,2,2,
                3,3,3,3,3,3,
                3,3,3,3,3,3),
               byrow = TRUE, ncol = 6)
            )
ddsjoberg commented 1 year ago

Let's hold off on this until the next release for this. I would hope to get p | p | p working properly if at all possible.

  1. Can we properly implement a |.ggplot method that could build and wrap the second argument as needed, then pass that object to the patchwork implementation of |.ggplot ?
  2. The issue here is trying to work with 2 different |.ggplot methods. If we did more reading perhaps we could implement our own |.ggplot methods, but pass those on to patchwork::wrap_plots() instead of taking advantage of the patchwork arithmetic operators. I am not sure if this would interact with the other functions that work with the operators exported from patchwork.

Oy!

michaelcurry1123 commented 1 year ago

complex issue! happy to help think through this with you for next release.

ddsjoberg commented 11 months ago

closing in favor of S7