Dynamic registration of themes

[teunbrand]

Hi there,

We've been preparing a new major release for ggplot2 and found an issue during a reverse dependency check of shared reverse dependencies. We then traced back the issue to ComplexUpset.

In essence what happened was that ComplexUpset gets build on CRAN's machines with a fixed upset_themes list. If ggplot2 then changes an internal about how themes work, the fixed theme list does not reflect the updates. The remedy for this would be to register the upset_themes list when the ComplexUpset package is loaded (instead of when it is build). This PR dynamically sets the themes, which would resolve the issue in our shared reverse dependencies.

Please note that I wasn't able to execute the test-examples.R file, and you may have to inspect the unit tests manually. Some changes in visual tests are expected, primary due to absent axis ticks no longer taking up space.

You can test your code with the development version of ggplot2 by installing it as follows:

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

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

Best wishes,
Teun

[teunbrand]

I see the tests fail, but to my eyes the failures are unrelated to the contents of this PR

[krassowski]

Most likely! I will try to review and merge over the upcoming weekend.

[teunbrand]

Lovely, many thanks in advance!

[teunbrand]

This is just a simple reminder.
I also believe that this PR would fix #213

[abichat]

Still get the size aesthetic for lines warning, but the error is fixed on my reprex! Thanks!

[teunbrand]

For those that come across this issue after the ggplot2 4.0.0 release and find this package or dependencies thereof broken, you can try the following:

pak::pak("krassowski/complex-upset#212")

[vjcitn]

Dear @krassowski this is to indicate strong interest that this PR gets merged. If you have any interest in finding alternate maintainers of this package, which is significant to the Bioconductor ecosystem, please comment.

[federicomarini]

Big +1 on @vjcitn 's comment - the PR is really release-ready and it would fix quite a number of packages currently failing on Bioc (and possibly CRAN too...).
Federico

[krassowski]

the PR is really release-ready

Yeah though, past few releases did not make it to CRAN as for various reasons the package was rejected like because the documentation was too large. Merging PR is easy getting it into CRAN so far was always more of a commitment. If anyone is interested in helping to fix the CI that would help a lot and I would happily review a PR.

[federicomarini]

Wow, "documentation being too large" sounds more like a wishlist item to me.
But maybe we are a bit spoilt so to say with Bioconductor's standards.

Is the CI os matrix intentionally using such an old version of R?
Because one thought is to possibly go at it with a tabula rasa and remake it fully.
In many of my packages, I tend to go with something like this: https://github.com/iSEE/iSEE/actions/runs/15418432332
i.e. less focused on the backwards compatibility, looking rather ahead into the devel cycle of Bioc

As a provocative thought, or possibly not just that: consider migrating ComplexUpset onto Bioc? I am sure that the additional hurdle of the release and devel branches to maintain would be very much superseded by the ease of updating upon need. @vjcitn is there someone else that went this way (or FWIW the other way around)?