Add bayesplot_theme_*() functions and remove startup theme_set() #149

malcolmbarrett · 2018-05-02T18:18:04Z

Closes #117

This PR adds bayesplot_theme_set(), bayesplot_theme_get(), bayesplot_theme_update(), and bayesplot_theme_replace(), which apply only to bayesplots. The user can change the themes applied to bayesplot using these functions, which are now the default for all plotting functions. bayesplot no longer changes the default ggplot2 theme, instead using this internal approach, and bayesplot_theme_*() functions do not affect other ggplots.

The ggplot2 equivalents supersede these. Loading ggplot2 will not affect bayesplot themes, but explicitly calling e.g. theme_set(theme_minimal()) will change all ggplots, including bayesplots, to theme_minimal(). I believe that this is the functionality most people would want, but this is done with a single line of code checking if the ggplot2 theme is set to anything other than theme_grey() (the default) and can easily be removed.

Closes stan-dev#257 Reflects changes to `bayesplot` approach to themes in stan-dev/bayesplot#149

codecov-io · 2018-05-02T19:51:40Z

Codecov Report

Merging #149 into master will decrease coverage by 0.23%.
The diff coverage is 90.98%.

@@            Coverage Diff             @@
##           master     #149      +/-   ##
==========================================
- Coverage   98.79%   98.55%   -0.24%     
==========================================
  Files          30       30              
  Lines        3891     3952      +61     
==========================================
+ Hits         3844     3895      +51     
- Misses         47       57      +10

Impacted Files	Coverage Δ
R/mcmc-combo.R	`96% <100%> (+0.16%)`	⬆️
R/ppc-errors.R	`99.52% <100%> (ø)`	⬆️
R/ppc-discrete.R	`98.85% <100%> (+0.01%)`	⬆️
R/ppc-loo.R	`100% <100%> (ø)`	⬆️
R/mcmc-parcoord.R	`100% <100%> (ø)`	⬆️
R/helpers-gg.R	`95.45% <100%> (ø)`	⬆️
R/ppc-scatterplots.R	`100% <100%> (ø)`	⬆️
R/ppc-intervals.R	`98.08% <100%> (+0.01%)`	⬆️
R/ppc-distributions.R	`100% <100%> (ø)`	⬆️
R/mcmc-distributions.R	`100% <100%> (ø)`	⬆️
... and 9 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 72ff752...3b498a5. Read the comment docs.

jgabry · 2018-05-02T21:46:12Z

@malcolmbarrett Thanks so much for working on this! We'll go through and review it as soon as possible.

tjmahr · 2018-05-03T13:30:46Z

In the final case, the bayesplot is still using the dark theme.

library(bayesplot)
library(ggplot2)

# plot using the default theme automatically 
x <- example_mcmc_draws()
mcmc_hist(x)

# change theme for all ggplots
theme_set(theme_dark())
mcmc_hist(x)

# change back to the default
bayesplot_theme_set(theme_default())
mcmc_hist(x)

tjmahr · 2018-05-03T13:44:50Z

In mcmc_intervals() and others, we do some light theming on the plot. For example, we move the legend to the top and bold the y-axis font and tick labels. Applying the current theme at the end undoes those theme changes.

  ggplot(data) +
    layer_vertical_line +
    layer_outer +
    layer_inner +
    layer_point +
    scale_color +
    scale_fill +
    scale_y_discrete(limits = unique(rev(data$parameter))) +
    xlim(x_lim) +
    legend_move(ifelse(color_by_rhat, "top", "none")) +
    yaxis_text(face = "bold") +
    yaxis_title(FALSE) +
    yaxis_ticks(size = 1) +
    xaxis_title(FALSE) +
    bayesplot_theme_get()

We should apply the bayesplot theme earlier so that the theming works. We should also limit/cut back on this kind of by-plot theming and document any custom-theming (that can be its own issue). (I think there are good uses for by-plot theming, like in the ridgeline plots where it's good have horizontal lines and to align the axis labels above the tick marks.)

malcolmbarrett · 2018-05-03T18:17:22Z

@tjmahr that example isn't in the docs, right? In that case, setting theme_set(theme_grey()) would reset it, but that's a little out of the way. I did think it was possible that there would be a small subset of people that would want different themes set for regular ggplots and bayesplots (that seems strange to me, but maybe I'm wrong), in which case it may make sense to have something like override_theme_set(TRUE) or something with options to always use bayesplot_theme_get() regardless of the set ggplot theme. That could be embedded in bayesplot_theme_set, e.g. bayesplot_theme_get(theme_default(), override = TRUE)... but it all seems like such a specific use case to me.

Regarding your second comment: I shuffled the theme calls around so they are now in the correct order.

tjmahr · 2018-05-03T19:01:01Z

I did think it was possible that there would be a small subset of people that would want different themes set for regular ggplots and bayesplots (that seems strange to me, but maybe I'm wrong), in which case it may make sense to have something like override_theme_set(TRUE) or something with options to always use bayesplot_theme_get() regardless of the set ggplot theme.

I'm thinking more in terms of the code working as expected... bayesplot_theme_set() should always update the bayesplot theme, even if theme_set() has already specified a global ggplot2 theme. (Edit: In that example code I had above, it did not work as I expected because the dark theme persists.)

malcolmbarrett · 2018-05-03T21:52:21Z

When you put it that way, I think you're absolutely right. Try this. It should work the way you describe. The only case I can come up with where it's not totally in order is something like this:

#  change all themes
theme_set(theme_dark())
mcmc_hist(x)

# change back to the default
bayesplot_theme_set(theme_default())
mcmc_hist(x)

# try to change all themes again to the same theme as above 
theme_set(theme_dark())
mcmc_hist(x) # still theme_default()

# but setting a new theme works:
theme_set(theme_minimal())
mcmc_hist(x) # theme_minimal()

This is because it now saves the existing ggplot2 theme_get() and compares it the next time bayesplot_get_theme() is run, but there's no way to tell that the second theme_dark() is different. Still, that's a bit of a silly situation, and if you really want it back to theme_dark(), you can do it with bayesplot_theme_set(). Thoughts?

jgabry · 2018-05-03T21:55:32Z

@malcolmbarrett Thanks again for this PR! I haven't had a chance to run the code yet (will do soon), but some quick comments:

I did think it was possible that there would be a small subset of people that would want different themes set for regular ggplots and bayesplots (that seems strange to me, but maybe I'm wrong)

I actually don't think it's that strange but that may be because I've seen every single bayesplot plot many many times. Some of them aren't as easily readable if certain kinds of themes are set. So I actually think there can be a good reason for maintaining a separate theme for bayesplot, although most plots will be fine regardless of the theme.

@tjmahr I also definitely agree with both of these other items you mentioned:

We should also limit/cut back on this kind of by-plot theming and document any custom-theming (that can be its own issue).

bayesplot_theme_set() should always update the bayesplot theme, even if theme_set() has already specified a global ggplot2 theme

jgabry · 2018-05-03T22:07:23Z

I know it would be slightly different behavior than ggplot2::theme_set(), but I think it would be nice if bayesplot_theme_set() defaulted to setting bayesplot::theme_default() instead of throwing an error if no argument is specified. Having to do bayesplot_theme_set(theme_default()) in order to reset the default seems unnecessary. Thoughts?

malcolmbarrett · 2018-05-03T23:45:24Z

Ok, I'm convinced! Try out the solution I committed above and see what you think.

Having to do bayesplot_theme_set(theme_default()) in order to reset the default seems unnecessary.

Makes sense to me! Just added.

jgabry · 2018-05-05T19:25:12Z

@malcolmbarrett Thanks for adding the default. I just made a few minor tweaks to the doc and some internal code, but looks good!

@tjmahr Any else you think should be changed before we merge this?

tjmahr · 2018-05-08T15:01:54Z

@jgabry Let me look at this Wednesday. I'll run the visual unit tests too. We should start to think about the ideas in #120 for our next steps.

jgabry · 2018-05-08T15:07:27Z

@tjmahr sounds good (and no rush if you’re super busy at the moment)

tjmahr · 2018-06-01T18:11:14Z

R/bayesplot-ggplot-themes.R

+#' mcmc_dens_overlay(x)
+#'
+bayesplot_theme_get <- function() {
+  if (!identical(.bayesplot_theme_env$gg_current, ggplot2::theme_get())) {


This is cool

tjmahr · 2018-06-01T18:22:34Z

R/mcmc-diagnostics-nuts.R

@@ -262,14 +264,16 @@ mcmc_nuts_divergence <- function(x, lp, chain = NULL, ...) {
  violin_lp <- ggplot(violin_lp_data, aes_(x = ~ Value, y = ~ lp)) +
    geom_violin(fill = get_color("l"), color = get_color("lh")) +
    ylab("lp__") +
-    xaxis_title(FALSE)
+    xaxis_title(FALSE) +
+      bayesplot_theme_get()


I had thought that bayesplot_theme_get() would override the change by xaxis_title() because xaxis_title() can theme xaxis titles. But xaxis_title(FALSE) just calls labs() so these are all fine.

jgabry · 2018-06-05T18:18:42Z

Just double checking, are we ready to finally merge this?

tjmahr · 2018-06-06T12:50:00Z

I think so!

@malcolmbarrett

Add @malcolmbarrett to DESCRIPTION file as a contributor and add these changes to the NEWS file.

jgabry

@malcolmbarrett Thanks again for doing this. Finally merging!

malcolmbarrett added 4 commits May 1, 2018 11:23

add bayesplot_theme_*() functions and remove startup theme change

83f38c7

add bayesplot_theme_get() to plotting functions

b50f09b

add bayetheme to ppc_test*()

81dc679

update docs

12d05ce

malcolmbarrett mentioned this pull request May 2, 2018

Remove startup ggplot2 theme set and add theme to plot functions stan-dev/rstanarm#258

Closed

make startup message a little clearer

e1f8a2c

malcolmbarrett added a commit to malcolmbarrett/rstanarm that referenced this pull request May 2, 2018

Remove theme_set() on startup and change message

ce43daf

Closes stan-dev#257 Reflects changes to `bayesplot` approach to themes in stan-dev/bayesplot#149

malcolmbarrett mentioned this pull request May 2, 2018

Remove theme_set() on startup and change message stan-dev/rstanarm#278

Merged

jgabry requested review from jgabry and tjmahr May 2, 2018 21:46

remove side effect line from documentation

fe2114e

malcolmbarrett added 2 commits May 3, 2018 08:12

fix order of theme elements

57109e3

Merge branch 'master' of https://github.com/malcolmbarrett/bayesplot

be485c9

make bayesplote_theme_set overpower theme_set

c51148f

change bayesplot_theme_set() to set theme_dafult() by default

48417de

Minor tweaks to doc and internals

f29b837

tjmahr reviewed Jun 1, 2018

View reviewed changes

tjmahr approved these changes Jun 1, 2018

View reviewed changes

Merge branch 'master' into master

0e8dd7a

Update DESCRIPTION and NEWS

3b498a5

Add @malcolmbarrett to DESCRIPTION file as a contributor and add these changes to the NEWS file.

jgabry approved these changes Jun 9, 2018

View reviewed changes

jgabry merged commit 8ba00fb into stan-dev:master Jun 9, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add bayesplot_theme_*() functions and remove startup theme_set() #149

Add bayesplot_theme_*() functions and remove startup theme_set() #149

malcolmbarrett commented May 2, 2018 •

edited

Loading

codecov-io commented May 2, 2018 •

edited

Loading

jgabry commented May 2, 2018

tjmahr commented May 3, 2018

tjmahr commented May 3, 2018

malcolmbarrett commented May 3, 2018

tjmahr commented May 3, 2018 •

edited

Loading

malcolmbarrett commented May 3, 2018

jgabry commented May 3, 2018

jgabry commented May 3, 2018 •

edited

Loading

malcolmbarrett commented May 3, 2018

jgabry commented May 5, 2018 •

edited

Loading

tjmahr commented May 8, 2018

jgabry commented May 8, 2018

tjmahr Jun 1, 2018

tjmahr Jun 1, 2018

jgabry commented Jun 5, 2018

tjmahr commented Jun 6, 2018

jgabry left a comment

Add bayesplot_theme_*() functions and remove startup theme_set() #149

Add bayesplot_theme_*() functions and remove startup theme_set() #149

Conversation

malcolmbarrett commented May 2, 2018 • edited Loading

codecov-io commented May 2, 2018 • edited Loading

Codecov Report

jgabry commented May 2, 2018

tjmahr commented May 3, 2018

tjmahr commented May 3, 2018

malcolmbarrett commented May 3, 2018

tjmahr commented May 3, 2018 • edited Loading

malcolmbarrett commented May 3, 2018

jgabry commented May 3, 2018

jgabry commented May 3, 2018 • edited Loading

malcolmbarrett commented May 3, 2018

jgabry commented May 5, 2018 • edited Loading

tjmahr commented May 8, 2018

jgabry commented May 8, 2018

tjmahr Jun 1, 2018

Choose a reason for hiding this comment

tjmahr Jun 1, 2018

Choose a reason for hiding this comment

jgabry commented Jun 5, 2018

tjmahr commented Jun 6, 2018

jgabry left a comment

Choose a reason for hiding this comment

malcolmbarrett commented May 2, 2018 •

edited

Loading

codecov-io commented May 2, 2018 •

edited

Loading

tjmahr commented May 3, 2018 •

edited

Loading

jgabry commented May 3, 2018 •

edited

Loading

jgabry commented May 5, 2018 •

edited

Loading