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

Plot arithmetic for ggsurvfit? #96

Open
ddsjoberg opened this issue Oct 2, 2022 · 11 comments
Open

Plot arithmetic for ggsurvfit? #96

ddsjoberg opened this issue Oct 2, 2022 · 11 comments

Comments

@ddsjoberg
Copy link
Collaborator

ddsjoberg commented Oct 2, 2022

Is it feasible to export methods for +.ggsurvfit, -.ggsurvfit, /.ggsurvfit, etc. They would work just like the patchwork versions, but would call ggsurvfit_build() to first construct the risktable, then pass the objects on to the ggplot2 methods.

https://patchwork.data-imaginist.com/reference/plot_arithmetic.html

I did some preliminary testing with a built ggsurvfit figure, and it did not seem to be as straight-forward as I was hoping.

This help file on generics for operators is helpful. The class of both sides of the operator are considered before it's dispatched to a method.
https://stat.ethz.ch/R-manual/R-devel/library/base/html/groupGeneric.html

@ddsjoberg

This comment was marked as outdated.

@ddsjoberg

This comment was marked as outdated.

@ddsjoberg ddsjoberg mentioned this issue Oct 9, 2022
14 tasks
@ddsjoberg

This comment was marked as outdated.

@ddsjoberg

This comment was marked as outdated.

@ddsjoberg
Copy link
Collaborator Author

@ddsjoberg
Copy link
Collaborator Author

# FROM HADLEY WICKHAM USING S7 PACKAGE
method(`|`, list(new_S3_class("ggsurvfit"), new_S3_class("ggsurvfit"))) <- function(e1, e2) { 
  
}
method(`|`, list(new_S3_class("ggsurvfit"), new_S3_class("ggplot"))) <- function(e1, e2) { 
  
}
method(`|`, list(new_S3_class("ggplot"), new_S3_class("ggsurvfit"))) <- function(e1, e2) { 

}

@ddsjoberg ddsjoberg added this to the Release v1.0.0 milestone Sep 30, 2023
@ddsjoberg
Copy link
Collaborator Author

ddsjoberg commented Oct 11, 2023

This works! Next steps:

  • re-write ggsurvfit() and ggcuminc() to be S7 class.
  • investigate how this change may affect methods that depend on the object's "ggplot" class, e.g. what is going to happen to +.ggplot!?
library(S7)
library(ggsurvfit)
#> Loading required package: ggplot2

ggsurvfit_S7 <- 
  new_class("ggsurvfit_S7", properties = list(obj = new_S3_class("ggsurvfit"))
)

method(`|`, list(ggsurvfit_S7, new_S3_class("ggplot"))) <- function(e1, e2) { 
  "This is ggsurvfit|ggplot"
}

method(`|`, list(new_S3_class("ggplot"), ggsurvfit_S7)) <- function(e1, e2) { 
  "This is ggplot|ggsurvfit"
}

method(`|`, list(ggsurvfit_S7, ggsurvfit_S7)) <- function(e1, e2) { 
  "This is ggsurvfit|ggsurvfit"
}



plot1 <- ggsurvfit_S7(
  obj = survfit2(Surv(time, status) ~ sex, data = df_lung) |> 
    ggsurvfit() +
    add_risktable()
)

plot2 <- ggplot(mtcars, aes(mpg, cyl)) + geom_point()

plot1 | plot2
#> [1] "This is ggsurvfit|ggplot"

plot2 | plot1
#> [1] "This is ggplot|ggsurvfit"

plot1 | plot1
#> [1] "This is ggsurvfit|ggsurvfit"


method(`|`, list(ggsurvfit_S7, ggsurvfit_S7)) <- function(e1, e2) { 
  patchwork::wrap_elements(ggsurvfit_build(e1@obj)) | patchwork::wrap_elements(ggsurvfit_build(e2@obj))
}
#> Overwriting method |(<ggsurvfit_S7>, <ggsurvfit_S7>)

plot1 | plot1

Created on 2023-10-10 with reprex v2.0.2

@ddsjoberg
Copy link
Collaborator Author

UPDATE FOR THE FUTURE DANIEL TO HOPEFULLY NOT FORGET!

  1. Implementing |.ggsurvfit methods does not work because it will only work correctly for 2 plots. A third plot would then be class ggplot | ggsurvfit and will fail
`|.ggsurvfit` <- function(e1, e2) {
  patchwork::wrap_elements(ggsurvfit_build(e1)) |
    patchwork::wrap_elements(ggsurvfit_build(e2))
}
  1. I have decided not to use S7, because I would just like the ggsurvfit objects to seamlessly use any S3 method defined for ggplots. If I were to make ggsurvift objects S7, I would need to re-implement all S3 methods meant for ggplot obejcts. While this is feasible for the methods exports by ggplot (notably, +.gg), any extension package that uses S3 methods would no longer work for ggsurvfit objects.

I am hopeful that patchwork will green-light the new generic function suggested in thomasp85/patchwork#310 . Thomas added the feature tag a couple of months ago, hopefully signaling he's open to the suggestion 🤞🏼

@ddsjoberg ddsjoberg removed this from the Release v1.0.0 milestone Oct 26, 2023
@teunbrand
Copy link

Hi Daniel,

I just watched your presentation on ggsurfvit and it was very well presented!
If this risk table is giving you grief with patchwork, I have an alternative idea that you might entertain for a bit.
The ggplot2 guide update is upon us, so when it hits the fan, why not implement the risk table as an axis guide?
I'm totally unqualified to opine on the risk table itself and what the specs are, but custom axes should just gently weave themselves into patchwork.

@ddsjoberg
Copy link
Collaborator Author

Ahhh, very interesting! I need to read up on the new features for the guides! Maybe we can set up a time to chat about the details soon?

I couple of potential issues come to mind:

  1. The numbers in the risk table must align with tick marks. Will it be tricky to to make that happen?
  2. We need to risktable to to remain below the figure. {patchwork} does an awesome job of gathering the guides for the treatment groups. Will it also try to gather up the risktables?

Anyway, would love to chat!

@teunbrand
Copy link

teunbrand commented Feb 3, 2024

The numbers in the risk table must align with tick marks. Will it be tricky to to make that happen?

Nah, you'd be surprised to learn that axis infrastructure is sorta geared towards aligning things at the tick marks 😅

We need to risktable to to remain below the figure. {patchwork} does an awesome job of gathering the guides for the treatment groups. Will it also try to gather up the risktables?

No, patchwork only mops up the non-position guides. Axes should remain in place.

Chat sounds great, just drop me an email if you want to propose dates/times!

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 a pull request may close this issue.

2 participants