-
Notifications
You must be signed in to change notification settings - Fork 632
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
Conversation
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
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 :)
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.
* 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 <175728472+Copilot@users.noreply.github.com> --------- Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
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
This is probably a side effect of {GGally} that overrides +.gg.
If you skip() the following test, these errors should no longer occur.
plotly.R/tests/testthat/test-plotly-subplot.R
Line 137 in 7a0176d
GGally probably needs to adapt to the new version too, but the C stack errors are not plotly's fault.
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