Skip to content
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

Compatibility with ggplot2 3.6.0 #337

Merged
merged 3 commits into from
Feb 4, 2025
Merged

Conversation

teunbrand
Copy link
Contributor

Hi there,

Apologies for not posting an issue first.
The ggplot2 package is planning an update for around May 2025 and a reverse dependency test identified a problem with the bayesplot package.
The details are explained in tidyverse/ggplot2#6290, but essentially ggplot2 doesn't populate the plot$labels field before plot building anymore, which violates some test assumptions in this package.

This PR changes the tests to be compatible with both versions of ggplot2.
In addition, some deprecated functionality is avoided.
You can test the changes yourself with the development version of ggplot2 (pak::pak("tidyverse/ggplot2")).
There are some visual changes as well, but I'll leave that up to you to decide wether intervention is needed.

Best,
Teun

@jgabry
Copy link
Member

jgabry commented Jan 30, 2025

Thanks for the PR!

@codecov-commenter
Copy link

codecov-commenter commented Jan 30, 2025

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 98.62%. Comparing base (20910f5) to head (abfc2ee).
Report is 8 commits behind head on master.

Additional details and impacted files
@@            Coverage Diff             @@
##           master     #337      +/-   ##
==========================================
+ Coverage   98.51%   98.62%   +0.11%     
==========================================
  Files          35       35              
  Lines        5034     5464     +430     
==========================================
+ Hits         4959     5389     +430     
  Misses         75       75              

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@jgabry jgabry linked an issue Feb 4, 2025 that may be closed by this pull request
@jgabry
Copy link
Member

jgabry commented Feb 4, 2025

Like @teunbrand mentioned, there are some visual changes that cause many of the vdiffr tests to fail. I haven't gone through all of them yet (there are a lot) but so far the changes seem small, not requiring intervention. So I'll go ahead and merge this PR and we will update the SVGs for the visual tests once the new version of ggplot2 is released.

@jgabry jgabry merged commit 7f45740 into stan-dev:master Feb 4, 2025
5 of 6 checks passed
@teunbrand
Copy link
Contributor Author

IIRC many of these changes relate to tidyverse/ggplot2#6071, which can be summarised as that blank ticks no longer reserve space for ticks, which affects the layout. If you really wanted to, you can render the ticks as transparent lines to retain the spacing, but it is a pretty benign change that doesn't ruin plots.

@teunbrand teunbrand deleted the ggplot2_360 branch February 4, 2025 16:53
@jgabry
Copy link
Member

jgabry commented Feb 4, 2025

Thanks for the context. I agree that the change seems benign.

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.

Move from size aesthetic to linewidth as size is deprecated.
3 participants