Skip to content

Conversation

@teunbrand
Copy link
Contributor

@teunbrand teunbrand commented Jun 18, 2025

Hi Daniel and team!

I bring tidings about the new ggplot2 version we have been preparing.
I'm perplexed to report that we didn't find any issues in ggsurvfit, but we did find some in shared reverse dependencies that we traced to ggsurvfit.
In addition to the changes proposed herein, you might want to verify visual tests yourself.
From my scuffle with these, it seems like some spacing is affected because we no longer reserve space for empty axis ticks and some legend orders have changed (which we might expect due to the 'secret algorithm' mentioned in ?guide_legend).

You can test your code with the development version of ggplot2 by installing it as follows:
(please note that tidyverse/ggplot2#6506 is intended to be merged in too, which will silence a series of warnings)

# install.packages("pak")
pak::pak("tidyverse/ggplot2")

We aim to release the new ggplot2 version in about 2 weeks, and hope you can submit an update to CRAN around that time. Hopefully this will inform you in a timely manner.

Best wishes,
Teun

What changes are proposed in this pull request?

  • removed element_text2(), it conflicted with ggplot2's new element classes
  • replaced object_name with ... in the arguments of ggplot_add() methods.

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

There is no such issue.


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

  • Ensure all package dependencies are installed by running renv::install()
  • PR branch has pulled the most recent updates from master branch. Ensure the pull request branch and your local version match and both have the latest updates from the master branch.
  • If a new function was added, function included in _pkgdown.yml
  • If a bug was fixed, a unit test was added for the bug check
  • Run pkgdown::build_site(). Check the R console for errors, and review the rendered website.
  • Overall code coverage remains >99.5%. Review coverage with withr::with_envvar(list(CI = TRUE), code = devtools::test_coverage()). Begin in a fresh R session without any packages loaded.
  • R CMD Check runs without errors, warnings, and notes
  • usethis::use_spell_check() runs with no spelling errors in documentation

When the branch is ready to be merged into master:

  • Update NEWS.md with the changes from this pull request under the heading "# ggsurvfit (development version)". If there is an issue associated with the pull request, reference it in parentheses at the end update (see NEWS.md for examples).
  • Increment the version number using usethis::use_version(which = "dev")
  • Run usethis::use_spell_check() again
  • Approve Pull Request
  • Merge the PR. Please use "Squash and merge".

@teunbrand
Copy link
Contributor Author

This is just a simple reminder.

@ddsjoberg
Copy link
Collaborator

the new ggplot2 version in about 2 weeks,

Thanks @teunbrand !!! Will review and merge now!

@ddsjoberg
Copy link
Collaborator

ddsjoberg commented Jul 24, 2025

Thanks again for the nudge @teunbrand !

Mostly everything works perfectly with the new ggplot. As you mentioned, there are some items that shifted slightly because ggplot no longer reserves space for empty axis ticks. This led to minor changes in snapshot tests...no biggie.

However, we are now seeing notes from the add_legend_title() function. In the example below, there is no fill or linetype aesthetics specified and we these notes "Ignoring unknown labels:" for the fill and linetype aesthetics. In the last example, I've added the a confidence interval, and now the legend does have a fill aesthetic and that note no longer appears. We need to update the logic in the function to only apply the legend title update for the types that are present to avoid the notes.

We'll need to investigate the new structure to see how we can avoid seeing these notes.

library(ggsurvfit)
#> Loading required package: ggplot2

# get get a note about FILL and LINETYPE
survfit2(Surv(time, status) ~ surg, data = df_colon) %>%
  ggsurvfit() +
  add_legend_title()
#> Ignoring unknown labels:
#> • fill : "Time from Surgery to Treatment"
#> • linetype : "Time from Surgery to Treatment"

# When the CI is present (and it does have a fill aestetic), the FILL note disappears
survfit2(Surv(time, status) ~ surg, data = df_colon) %>%
  ggsurvfit() +
  add_confidence_interval() +
  add_legend_title()
#> Ignoring unknown labels:
#> • linetype : "Time from Surgery to Treatment"

Created on 2025-07-23 with reprex v2.1.1

@teunbrand
Copy link
Contributor Author

Yes I recall these messages being thrown around a lot in ggsurvfit. It's the reason why I've demoted them from warnings to messages. The messages inform folks when they have overspecified labels: when labels are present but the aesthetics are not.
Would you like me to try to add a solution to prevent these messages to this PR?

@ddsjoberg
Copy link
Collaborator

@teunbrand ! Thank you so much for the offer! I imagine your bandwidth is a bit low to support every package using ggolot, so @ShreyaSreeram27 and I will have a stab at it first and we'll let you know if we get stuck?

Copy link
Collaborator

@ddsjoberg ddsjoberg left a comment

Choose a reason for hiding this comment

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

Thank you @teunbrand for the PR! Apologies for the delay on the review.

Also, thank you @ShreyaSreeram27 !

@ddsjoberg ddsjoberg merged commit 9129fde into pharmaverse:main Jul 25, 2025
9 of 10 checks passed
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.

3 participants