Skip to content

Compatibility ggplot2 4.0.0 #2440

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

Closed
wants to merge 3 commits into from

Conversation

teunbrand
Copy link
Contributor

Hi Carson,

We're preparing a new major version of ggplot2 and during a revdepcheck we bumped into a few things we think are best adressed in plotly rather than ggplot2.
I'm still getting a few node stack errors that I can't trace to ggplot2 or reproduce when re-running the relevant test file, so perhaps it'll be false alarm.
We plan to release the new ggplot2 version soon, so it'd be great if a compatible version of plotly was already on CRAN.

Best wishes,
Teun

@teunbrand
Copy link
Contributor Author

The following error:

Failure (test-plotly.R:145:3): Character strings correctly mapped to a positional axis
expect_traces(p, 2, "character-axis") did not produce any warnings.

I also encounter with CRAN version of ggplot2, so it is unlikely due to changes in the new version

cpsievert added a commit that referenced this pull request Jun 5, 2025
@cpsievert
Copy link
Collaborator

cpsievert commented Jun 5, 2025

Hey @teunbrand, thanks for this! Turns out there were a few more issues that the visual tests surfaced -- I'll bundle this fix as well as those other things in #2442.

PS. I'd be very happy to add you to authors at this point if you want to send another PR :)

@cpsievert cpsievert closed this Jun 5, 2025
@teunbrand
Copy link
Contributor Author

Ah the visual tests didn't run locally on my machine, so I was blind for any changes therein (I apologise!) I'll have to mention that the most invasive change we plan for 4.0.0 is not merged into the main branch yet. That change is ggplot2/#6364, and we're polishing that to minimise reverse dependency breakage. So if you're testing against dev ggplot2, then it'd be great if you could test against that PR.

cpsievert added a commit that referenced this pull request Jun 5, 2025
* Run tests against dev ggplot2

* Take changes from #2440

* Approve (a subset of) new snapshots

* Ensure ticktext/tickvals is atomic character/numeric vector

* Fix geom_sf() break positions

* Fix for geom_violin()

* Update news

* Approve good visual changes

* Approve more visual changes; fix test expectation

* Accept new snapshot

* Zero length ticktext/tickvals should remain NULL

* Zero length vs NULL can actually be meaningful so just safe guard against error

* Approve new snapshot

* Refactor

* Update NEWS.md

Co-authored-by: Copilot <[email protected]>

---------

Co-authored-by: Copilot <[email protected]>
@cpsievert
Copy link
Collaborator

Taking a quick peak, I see some stack overflows when axis ticks are customized. I'll have to investigate at a later time, and will probably wait until after that PR is merged.

── Error ('test-rotated-ticks.R:46:1'): no axis rotation is fine ───────────────
<CStackOverflowError/stackOverflowError/error/condition>
Error: C stack usage  7956064 is too close to the limit
── Error ('test-rotated-ticks.R:51:1'): axis.text.x=element_text(angle=90)" ────
<CStackOverflowError/stackOverflowError/error/condition>
Error: C stack usage  7956064 is too close to the limit
── Error ('test-rotated-ticks.R:57:1'): axis.text.x=element_text(angle=70) means transform="rotate(-70)" ──
<CStackOverflowError/stackOverflowError/error/condition>
Error: C stack usage  7956064 is too close to the limit
── Error ('test-ticktext-linebreaks.R:32:1'): ggplotly takes account of linebreaks in ticktext ──
<CStackOverflowError/stackOverflowError/error/condition>
Error: C stack usage  7955584 is too close to the limit
── Error ('test-ticktext-linebreaks.R:43:1'): ggplotly takes account of linebreaks in ticktext with only one category ──
<CStackOverflowError/stackOverflowError/error/condition>
Error: C stack usage  7955584 is too close to the limit
── Error ('test-ticktext-linebreaks.R:54:1'): ggplotly works with no linebreaks in ticktext ──
<CStackOverflowError/stackOverflowError/error/condition>
Error: C stack usage  7955584 is too close to the limit

@teunbrand
Copy link
Contributor Author

This is probably a side effect of {GGally} that overrides +.gg.
If you skip() the following test, these errors should no longer occur.

test_that("ggplotly understands GGally", {

GGally probably needs to adapt to the new version too, but the C stack errors are not plotly's fault.

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.

2 participants