krassowski / complex-upset

A library for creating complex UpSet plots with ggplot2 geoms
MIT License
469 stars 28 forks source link

API seems broken after the latest updates in ggplot2 (>= 3.5.0) #195

Open federicomarini opened 9 months ago

federicomarini commented 9 months ago

Describe the bug

After the latest changes introduced by ggplot2 3.5.0, the API for upset seems broken.

I noticed this behavior as this is triggered in the Bioconductor Build System for my package GeneTonic -> https://master.bioconductor.org/checkResults/3.19/bioc-LATEST/GeneTonic/nebbiolo1-checksrc.html

Code to reproduce

## taken from your vignette
library(ggplot2)
library(ComplexUpset)
movies = as.data.frame(ggplot2movies::movies)
head(movies, 3)
genres = colnames(movies)[18:24]
genres
movies[genres] = movies[genres] == 1
t(head(movies[genres], 3))
movies[movies$mpaa == '', 'mpaa'] = NA
movies = na.omit(movies)
upset(movies, genres, name='genre', width_ratio=0.1)
sessionInfo()

Expected behavior

The plot is not generated, and the screenshot below is generated instead.

> upset(movies, genres, name='genre', width_ratio=0.1)
Error in `plot_theme()`:
! The `legend.text.align` theme element is not defined in the element
  hierarchy.
Run `rlang::last_trace()` to see where the error occurred.

Screenshots

image

Context (required)

ComplexUpset version: 1.3.5

R version details ```R > R.Version() $platform [1] "x86_64-apple-darwin20" $arch [1] "x86_64" $os [1] "darwin20" $system [1] "x86_64, darwin20" $status [1] "" $major [1] "4" $minor [1] "3.0" $year [1] "2023" $month [1] "04" $day [1] "21" $`svn rev` [1] "84292" $language [1] "R" $version.string [1] "R version 4.3.0 (2023-04-21)" $nickname [1] "Already Tomorrow" ```
R session information ```R > sessionInfo() R version 4.3.0 (2023-04-21) Platform: x86_64-apple-darwin20 (64-bit) Running under: macOS Monterey 12.7.1 Matrix products: default BLAS: /System/Library/Frameworks/Accelerate.framework/Versions/A/Frameworks/vecLib.framework/Versions/A/libBLAS.dylib LAPACK: /Library/Frameworks/R.framework/Versions/4.3-x86_64/Resources/lib/libRlapack.dylib; LAPACK version 3.11.0 locale: [1] en_US.UTF-8/en_US.UTF-8/en_US.UTF-8/C/en_US.UTF-8/en_US.UTF-8 time zone: Europe/Berlin tzcode source: internal attached base packages: [1] stats4 stats graphics grDevices utils datasets methods [8] base other attached packages: [1] ComplexUpset_1.3.5 ggplot2_3.5.0 [3] org.Hs.eg.db_3.18.0 AnnotationDbi_1.64.1 [5] DESeq2_1.42.0 SummarizedExperiment_1.32.0 [7] Biobase_2.62.0 MatrixGenerics_1.14.0 [9] matrixStats_1.2.0 GenomicRanges_1.54.1 [11] GenomeInfoDb_1.38.5 IRanges_2.36.0 [13] S4Vectors_0.40.2 BiocGenerics_0.48.1 [15] macrophage_1.18.0 GeneTonic_2.7.2 [17] testthat_3.2.1 loaded via a namespace (and not attached): [1] RColorBrewer_1.1-3 rstudioapi_0.15.0 jsonlite_1.8.8 [4] shape_1.4.6 magrittr_2.0.3 rmarkdown_2.25 [7] farver_2.1.1 GlobalOptions_0.1.2 fs_1.6.3 [10] zlibbioc_1.48.0 vctrs_0.6.5 memoise_2.0.1 [13] RCurl_1.98-1.14 htmltools_0.5.7 S4Arrays_1.2.0 [16] usethis_2.2.2 dynamicTreeCut_1.63-1 SparseArray_1.2.3 [19] sass_0.4.8 bslib_0.6.1 htmlwidgets_1.6.4 [22] desc_1.4.3 plotly_4.10.4 cachem_1.0.8 [25] igraph_2.0.1.1 mime_0.12 lifecycle_1.0.4 [28] iterators_1.0.14 pkgconfig_2.0.3 colourpicker_1.3.0 [31] Matrix_1.6-5 R6_2.5.1 fastmap_1.1.1 [34] GenomeInfoDbData_1.2.11 shiny_1.8.0 clue_0.3-65 [37] digest_0.6.34 colorspace_2.1-0 shinycssloaders_1.0.0 [40] patchwork_1.2.0 rprojroot_2.0.4 pkgload_1.3.4 [43] RSQLite_2.3.5 labeling_0.4.3 fansi_1.0.6 [46] httr_1.4.7 polyclip_1.10-6 abind_1.4-5 [49] compiler_4.3.0 remotes_2.4.2.1 bit64_4.0.5 [52] withr_3.0.0 doParallel_1.0.17 BiocParallel_1.36.0 [55] viridis_0.6.5 DBI_1.2.1 shinyAce_0.4.2 [58] dendextend_1.17.1 pkgbuild_1.4.3 ggforce_0.4.1 [61] MASS_7.3-60.0.1 DelayedArray_0.28.0 sessioninfo_1.2.2 [64] rjson_0.2.21 tools_4.3.0 httpuv_1.6.14 [67] tippy_0.1.0 glue_1.7.0 promises_1.2.1 [70] grid_4.3.0 bs4Dash_2.3.3 cluster_2.1.6 [73] generics_0.1.3 gtable_0.3.4 tidyr_1.3.1 [76] data.table_1.15.0 utf8_1.2.4 XVector_0.42.0 [79] ggrepel_0.9.5 foreach_1.5.2 pillar_1.9.0 [82] stringr_1.5.1 later_1.3.2 rintrojs_0.3.4 [85] circlize_0.4.15 dplyr_1.1.4 tweenr_2.0.2 [88] lattice_0.22-5 bit_4.0.5 tidyselect_1.2.0 [91] GO.db_3.18.0 ComplexHeatmap_2.18.0 locfit_1.5-9.8 [94] Biostrings_2.70.2 miniUI_0.1.1.1 knitr_1.45 [97] gridExtra_2.3 ggplot2movies_0.0.1 xfun_0.41 [100] expm_0.999-9 brio_1.1.4 devtools_2.4.5 [103] DT_0.31 visNetwork_2.1.2 stringi_1.8.3 [106] lazyeval_0.2.2 shinyWidgets_0.8.1 evaluate_0.23 [109] codetools_0.2-19 tibble_3.2.1 cli_3.6.2 [112] xtable_1.8-4 jquerylib_0.1.4 munsell_0.5.0 [115] Rcpp_1.0.12 png_0.1-8 parallel_4.3.0 [118] ellipsis_0.3.2 blob_1.2.4 profvis_0.3.8 [121] urlchecker_1.0.1 bitops_1.0-7 viridisLite_0.4.2 [124] scales_1.3.0 backbone_2.1.3 ggridges_0.5.6 [127] purrr_1.0.2 crayon_1.5.2 GetoptLong_1.0.5 [130] rlang_1.1.3 KEGGREST_1.42.0 ```
federicomarini commented 9 months ago

Thanks in advance for looking into this!

smped commented 9 months ago

Thanks for flagging this so quickly @federicomarini . Can also confirm that two of my packages which depend on ComplexUpset are also failing to build on Bioconductor with the same error, i.e.

The `legend.text.align` theme element is not defined in the element hierarchy

https://master.bioconductor.org/checkResults/3.18/bioc-LATEST/extraChIPs/nebbiolo2-buildsrc.html

smped commented 9 months ago

Ok, I think this is a curly one. I ran the following code, noting that upset_default_themes() simply calls theme_minimal(). Testing this showed that the call to theme_minimal() performed by upset_default_themes() returned a version of theme_minimal() from ggplot2 v3.4.x, not 3.5.0, even though I have ggplot2 v3.5.0 installed.

setdiff(names(ComplexUpset::upset_default_themes()$default[[1]]), names(formals(theme)))

[1] "legend.text.align" "legend.title.align"

I wondered if this meant ComplexUpset had been installed from a binary so I ran remove.packages("ComplexUpset") and reinstalled using install.packages("ComplexUpset", type = "source"). Now the conflict has resolved so I think we need to remove any cached packages from github actions and I'll also check to see if ComplexUpset can be re-installed on the Bioconductor Build System.

federicomarini commented 9 months ago

Well done in investigating this a bit. Potentially, the GHA system should be able to "catch these things" in case newer package versions come along. And the Bioc BBS is actually always picking the latest versions. I'll keep this monitored in the coming days, as we are somehow approaching high noon (= the upcoming release deadlines).

smped commented 9 months ago

Thanks. I can't see the BBS picking this up as there's no new version of ComplexUpset to trigger a reinstallation of ComplexUpset. The currently installed version will continue to return the incompatible values from it's call to theme_minimal() until it's reinstalled on a system after ggplot2 v3.5.0 has been installed

lisiarend commented 9 months ago

Ok, I think this is a curly one. I ran the following code, noting that upset_default_themes() simply calls theme_minimal(). Testing this showed that the call to theme_minimal() performed by upset_default_themes() returned a version of theme_minimal() from ggplot2 v3.4.x, not 3.5.0, even though I have ggplot2 v3.5.0 installed.

setdiff(names(ComplexUpset::upset_default_themes()$default[[1]]), names(formals(theme)))

[1] "legend.text.align" "legend.title.align"

I wondered if this meant ComplexUpset had been installed from a binary so I ran remove.packages("ComplexUpset") and reinstalled using install.packages("ComplexUpset", type = "source"). Now the conflict has resolved so I think we need to remove any cached packages from github actions and I'll also check to see if ComplexUpset can be re-installed on the Bioconductor Build System.

I could not get it fixed with your solution.

lshep commented 9 months ago

The Bioconductor Build System would like to avoid having to uninstall and reinstall a package manually since this hides what a user sees on their local system. A user with the previously installed would also have to be aware to uninstall and reinstall manually. Ideally ComplexUpset would bump their version forcing a new version install for all users universally if this is indeed all that needed to be done.

federicomarini commented 9 months ago

if this is indeed all that needed to be done

And if this is all, well, I think we can already prep up a "close to fake" PR to have the new version picked up by the related systems 😅

federicomarini commented 9 months ago

Ok, I think this is a curly one. I ran the following code, noting that upset_default_themes() simply calls theme_minimal(). Testing this showed that the call to theme_minimal() performed by upset_default_themes() returned a version of theme_minimal() from ggplot2 v3.4.x, not 3.5.0, even though I have ggplot2 v3.5.0 installed.

setdiff(names(ComplexUpset::upset_default_themes()$default[[1]]), names(formals(theme)))

[1] "legend.text.align" "legend.title.align" I wondered if this meant ComplexUpset had been installed from a binary so I ran remove.packages("ComplexUpset") and reinstalled using install.packages("ComplexUpset", type = "source"). Now the conflict has resolved so I think we need to remove any cached packages from github actions and I'll also check to see if ComplexUpset can be re-installed on the Bioconductor Build System.

I could not get it fixed with your solution.

FWIW, the suggested solution from @smped worked in my hands.

Uninstalling explicitly, installing (from source, dunno if that really helps, but hey, it worked), re-running the example and the functions from my package using that: ✅

smped commented 9 months ago

Thanks @lshep & @federicomarini . I've added a bug catcher to my function so I think I'm OK with this one, but it did seem a bit odd. I'm passing arguments through the dots which I have as dotArgs

valid_theme_args <- names(formals(theme))
  dotArgs$themes <- lapply(
      dotArgs$themes,
      \(x) lapply(x, \(y) do.call("theme", y[names(y) %in% valid_theme_args]))
  )

Sorry to hear your attempt didn't work either @lisiarend. Hopefully the package authors will be able to help.

jctourtellotte commented 9 months ago

So.... will uninstalling and reinstalling fix this? I am going to try that, because I am also having this issue and I'm mid-dissertation with the use of an UpSet plot being fairly significant in one chunk of the analysis I'm writing up. Is there a nail biting emoji?

Edit: uninstalling and reinstalling from source (install.packages("ComplexUpset", type = "source")) worked. The from source was key, as I had tried to uninstall/reinstall through the RStudio GUI and that didn't work.

federicomarini commented 9 months ago

Glad it worked @jctourtellotte ! The issue still persists for (build) systems where this should likely be less triggered by hand, but rather picked up automatically. My guess is that a simple version bump could do it.

If that did not solve it, you could ideally revert back to the previous ggplot2 version. And/or you can protect yourself from such things with anything on the line of renv to keep package collection snapshots.

adrientaudiere commented 8 months ago

Hi all, my R package depends on ComplexUpset package and I have an error from CRAN which seems very similar to the problem you are talking about in this issue (link to CRAN logs. I put an extract of the error below. CRAN team asked me to resolve this error. How can I fix in this special case?

[...]
Error(s) in re-building vignettes:
--- re-building ‘MiscMetabar.Rmd’ using rmarkdown

Quitting from lines 68-69 [unnamed-chunk-4] (MiscMetabar.Rmd)
Error: processing vignette 'MiscMetabar.Rmd' failed with diagnostics:
The `legend.text.align` theme element is not defined in the element
hierarchy.
--- failed re-building ‘MiscMetabar.Rmd’
[...]
federicomarini commented 8 months ago

Hm, not sure you can solve this, "alone". Since you do depend on this package, there's not too much you can do.

Maybe you can prevent your package from being archived on short term is to point towards this discussion, in the hope they understand it really does not relate to a mishap on your end @adrientaudiere ?

krassowski commented 8 months ago

So what specifically should this package do?

federicomarini commented 8 months ago

You mean ComplexUpset?

I think the solution proposal that @smped was pointing to is that you Michal would simply issue a new release with a version bump. I know it does not sound too complex, but it seemed to have done the job. I guess the fails on the CRAN and Bioc Build systems is due to the fact of

I also could not find any better solution so far, but it is worth a try from my point of view.

smped commented 8 months ago

Hi @krassowski ,

I think the real issue has been caused by a quirk in the R ecosystem & the decision within the tidyverse developer community to not enforce backwards compatibility.

The simplest fix to me is to push out a version bump which enforces a dependency on ggplot2 v3.5.0, and that should resolve this specific error, although who knows what may be coming in the future. So just a simple change the DESCRIPTION file & everything should be fine from our end.

The catch may be in the tests. I forked & tried the above in order to make a pull request, but all tests in test-other-visual.R failed for me & I didn't have time to figure them all out. And there's the ggplot2 deprecation of aes_ and aes_string() to contend with. (...sigh...). FWIW, I think I lose a week or two every year trying to fix my packages after the tidyverse breaks a bunch of things. Brilliant packages for users, terrible for developers.

adrientaudiere commented 8 months ago

Thanks @smped and @federicomarini for your comprehensive answer. Maybe I can enforce a dependency on ggplot2 v3.5.0 in my own package DESCRIPTION file. @krassowski do you plan to make a new release enforcing ggplot2 v3.5.0 soon?

krassowski commented 8 months ago

Yes, I can do that over the weekend.

federicomarini commented 8 months ago

Very appreciated, Mike, thanks!

smped commented 8 months ago

Brilliant. Thanks @krassowski . Much appreciated indeed

adrientaudiere commented 8 months ago

Oh yes, Thanks a lot @krassowski!

krassowski commented 8 months ago

Hi folks, I had submitted ComplexUpset 1.3.6 to CRAN. Fingers crossed.

krassowski commented 8 months ago

It appears that the submission to CRAN was rejected via an automated email because CRAN's link checker thinks that "https://anaconda.org/conda-forge/r-complexupset" does not work (but it does - you can check it yourself):

Dear maintainer,

package ComplexUpset_1.3.6.tar.gz does not pass the incoming checks automatically, please see the following pre-tests: Windows: https://win-builder.r-project.org/incoming_pretest/ComplexUpset_1.3.6_20240309_162121/Windows/00check.log Status: 1 NOTE Debian: https://win-builder.r-project.org/incoming_pretest/ComplexUpset_1.3.6_20240309_162121/Debian/00check.log Status: 1 NOTE

Last released version's CRAN status: OK: 12 See: https://cran.r-project.org/web/checks/check_results_ComplexUpset.html

CRAN Web: https://cran.r-project.org/package=ComplexUpset

Please fix all problems and resubmit a fixed version via the webform.

It is not an error, not even a warning. It is just a note...

The 00check.log contents:

* using log directory 'd:/RCompile/CRANincoming/R-devel/ComplexUpset.Rcheck'
* using R Under development (unstable) (2024-03-08 r86072 ucrt)
* using platform: x86_64-w64-mingw32
* R was compiled by
    gcc.exe (GCC) 12.3.0
    GNU Fortran (GCC) 12.3.0
* running under: Windows Server 2022 x64 (build 20348)
* using session charset: UTF-8
* checking for file 'ComplexUpset/DESCRIPTION' ... OK
* checking extension type ... Package
* this is package 'ComplexUpset' version '1.3.6'
* package encoding: UTF-8
* checking CRAN incoming feasibility ... [19s] NOTE
Maintainer: 'Michał Krassowski <krassowski.michal+r@gmail.com>'

Package CITATION file contains call(s) to old-style citEntry().  Please
use bibentry() instead.

Found the following (possibly) invalid URLs:
  URL: https://anaconda.org/conda-forge/r-complexupset
    From: README.md
    Status: 400
    Message: Bad Request

Size of tarball: 5107480 bytes
* checking package namespace information ... OK
* checking package dependencies ... OK
* checking if this is a source package ... OK
* checking if there is a namespace ... OK
* checking for hidden files and directories ... OK
* checking for portable file names ... OK
* checking serialization versions ... OK
* checking whether package 'ComplexUpset' can be installed ... OK
* checking installed package size ... OK
* checking package directory ... OK
* checking for future file timestamps ... OK
* checking 'build' directory ... OK
* checking DESCRIPTION meta-information ... OK
* checking top-level files ... OK
* checking for left-over files ... OK
* checking index information ... OK
* checking package subdirectories ... OK
* checking R files for non-ASCII characters ... OK
* checking R files for syntax errors ... OK
* checking whether the package can be loaded ... OK
* checking whether the package can be loaded with stated dependencies ... OK
* checking whether the package can be unloaded cleanly ... OK
* checking whether the namespace can be loaded with stated dependencies ... OK
* checking whether the namespace can be unloaded cleanly ... OK
* checking loading without being on the library search path ... OK
* checking use of S3 registration ... OK
* checking dependencies in R code ... OK
* checking S3 generic/method consistency ... OK
* checking replacement functions ... OK
* checking foreign function calls ... OK
* checking R code for possible problems ... OK
* checking Rd files ... OK
* checking Rd metadata ... OK
* checking Rd line widths ... OK
* checking Rd cross-references ... OK
* checking for missing documentation entries ... OK
* checking for code/documentation mismatches ... OK
* checking Rd \usage sections ... OK
* checking Rd contents ... OK
* checking for unstated dependencies in examples ... OK
* checking installed files from 'inst/doc' ... OK
* checking files in 'vignettes' ... OK
* checking examples ... OK
* checking for unstated dependencies in vignettes ... OK
* checking package vignettes ... OK
* checking re-building of vignette outputs ... OK
* checking PDF version of manual ... [13s] OK
* checking HTML version of manual ... OK
* DONE
Status: 1 NOTE
smped commented 8 months ago

Gosh. That seems really weird to get a fail based an a URL in a README!

Thanks for working in this over your weekend too & hopefully there's an easy fix.

federicomarini commented 8 months ago

I know it is annoying @krassowski , thanks for taking care of this so far.

Maybe you can argue that with one email back&forth? I recall once I had an issue (similar, not same) because they did not want me to leave a URL that was redirecting to our institute, but only accepted the direct link. But this does not seem to be the case here...

The bibentry() thing is something you could easily solve?

adrientaudiere commented 8 months ago

Thanks @krassowski. I was able to resubmit my package by forcing the ggplot2 version in the DESCRIPTION file of my package MiscMetabar.

Concerning the url, sometimes, one site may be dead for a little time and that may arise when CRAN is checking your package. This has already happened to me, but for a link to a much less stable site than anaconda.org.

Its is strange, especially as the DESCRIPTION file has not changed. I think if you solve the bibentry() note you can resubmit to CRAN adding in the field Optional comment: that you are surprised by the note about the url.

smped commented 7 months ago

Hi again @krassowski,

Just following up on this given the pending Bioconductor release. Have you managed to make any progress with CRAN?

Thanks