Infer annotation aesthetics from the active ggplot2 theme (#120)#451
Infer annotation aesthetics from the active ggplot2 theme (#120)#451ishaan-arora-1 wants to merge 4 commits intostan-dev:masterfrom
Conversation
Reference lines (e.g. the vertical zero-line in mcmc_intervals, the rhat=1 line in mcmc_rhat) previously used hardcoded colors like "gray90" that don't adapt when users switch ggplot2 themes. Added an internal annotation_style() helper that reads the active theme's gridline color and linewidth. When gridlines are present, the reference line inherits their color at double the major gridline width. When gridlines are blank (bayesplot's default), it falls back to the previous gray90/0.5 defaults so existing plots are unchanged. Closes stan-dev#120
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #451 +/- ##
==========================================
+ Coverage 98.55% 98.67% +0.12%
==========================================
Files 35 35
Lines 5864 5886 +22
==========================================
+ Hits 5779 5808 +29
+ Misses 85 78 -7 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
jgabry
left a comment
jgabry
left a comment
There was a problem hiding this comment.
Thanks @ishaan-arora-1. Just made several review comments.
- Revert geom_ignore() example to keep it simple (no annotation_style call) - annotation_style() now checks panel.grid.major.x / minor.x first, then falls back to panel.grid.major / minor, so themes that only customise vertical gridlines are handled correctly - Accept fallback_color and fallback_linewidth arguments so callers can preserve their original hardcoded defaults when gridlines are blank - Make all diagnostic reference lines in mcmc_rhat() and mcmc_neff() theme-aware (including the dashed break lines, not just the rhat=1 line) - Add unit tests for annotation_style() covering fallback values, theme reading, and panel.grid.major.x preference - Add vdiffr visual tests for mcmc_intervals, mcmc_rhat, and mcmc_neff under theme_gray() to verify theme-aware annotation rendering
|
Hey @jgabry how do the changes look now? |
The vdiffr snapshot for mcmc_neff with theme_gray() had minor sub-pixel coordinate drift after merging latest master. Accepted the new rendering — no visual change in the plot.
There was a problem hiding this comment.
Thinking more about this, I think we would need some before and after comparisons of the different cases this is supposed to benefit to actually see if this branch makes the plots easier to read. It's impossible to tell just from looking at the code. When I play around with it I think for mcmc_intervals() it looks better on this branch but for the diagnostic plots it's actually harder to see the lines using this branch (see, for example, the SVG snapshot you added for mcmc_neff). But we really need to see side by side comparisons to make an informed decision.
However, I'm honestly not sure how much work we want to put into this since this issue hasn't come up for many years. I'm not sure it's worth the effort and new code additions. But I'm open to reconsidering.
(I added a few more review comments, but it might not be worth it to work on them. It depends if these changes are actually beneficial.)
| # grid element is blank or NULL (i.e. the theme hides gridlines). Different | ||
| # plots historically used different hardcoded values, so callers can preserve | ||
| # backward-compatible defaults. | ||
| annotation_style <- function(fallback_color = "gray90", |
There was a problem hiding this comment.
annotation_style() should also fall back when the gridline is effectively invisible (color is NA or zero linewidth), not just when it is element_blank()/NULL. Right now if color is NA (which is possible) or the linewidth is 0 those themes will produce invisible annotations.
| bayesplot_theme_set(ggplot2::theme_gray()) | ||
| on.exit(bayesplot_theme_set(), add = TRUE) | ||
|
|
||
| rhats <- seq(from = 1, to = 1.20, length.out = 10) |
There was a problem hiding this comment.
The solid reference line at x = 1 is only added when min(data$value) < 1. With this test data, min = 1, so the solid rhat = 1 line never appears.
3 participants
Fixes #120.
Reference lines like the vertical zero-line in
mcmc_intervals()and the rhat=1 line inmcmc_rhat()had hardcoded colors ("gray90","gray") that didn't adapt when users switched ggplot2 themes. As @tjmahr pointed out in the issue, this makes them invisible or look wrong on themes liketheme_grey()or dark themes.Added an internal
annotation_style()helper that reads the active theme's gridline color and linewidth to derive sensible reference line aesthetics. When gridlines are blank (bayesplot's default theme), it falls back to the previousgray90/0.5defaults so existing plots look exactly the same.Updated the four internal call sites in
mcmc-intervals.R(3xvline_0) andmcmc-diagnostics.R(1xvline_at) to use the new helper.