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

Make the bayesplot default theme optional? #87

Closed
gavinsimpson opened this issue Apr 23, 2017 · 6 comments
Closed

Make the bayesplot default theme optional? #87

gavinsimpson opened this issue Apr 23, 2017 · 6 comments

Comments

@gavinsimpson
Copy link

At the moment, from what I can tell having only had a brief look at the code, you are hard coding thebayesplot theme_default() ggplot theme when each plot is created. This makes it tedious to override by the user (they must add their own + theme_foo() layers to every plot they create). Further, from a previous Issue I understand this might not work in all cases in bayesplot as some plots are actually multiple plots arranged on the device.

Instead, you can use the theme_set() functionality of ggplot2 to set the default theme when the package is attached. The cowplot package, for example, uses this to set its default theme. This requires an .onAttach() function to be defined in the package R source somewhere with the following:

.onAttach <- function(libname, pkgname) {
  ggplot2::theme_set(bayesplot::theme_default())
}

All instances of + theme_default() would also need to be removed.

Doing this won't change the output from plots unless a user decides to change the theme used with by their own explicit call to theme_set() after bayesplot is loaded.

@jgabry
Copy link
Member

jgabry commented Apr 24, 2017 via email

@gavinsimpson
Copy link
Author

@jgabry Yes, you are right; setting the theme globally as I suggested will affect every plot made with ggplot unless the plot code explicitly overrides the theme. However, that's easily and trivially fixed by having the user call theme_set().

If people have the pkg loaded (i.e. it's attached not just being imported from) it's probably because they want to use the package, so the number of people affected by my suggested change is likely low.

On the other hand the solution used in bayesplot isn't easy to work around; a user has to add + theme() etc to each plot, if they can affect the theme at all.

If you were to flag the issue, as cowplot does in documentation and in the README etc, I doubt this change would cause too much grief, but it would make modifying the look and feel of the plots trivial.

@jgabry
Copy link
Member

jgabry commented Apr 24, 2017 via email

gavinsimpson added a commit to gavinsimpson/bayesplot that referenced this issue Apr 25, 2017
@jgabry
Copy link
Member

jgabry commented Jul 24, 2017

PR #95 merged so closing this

@jgabry jgabry closed this as completed Jul 24, 2017
@gavinsimpson
Copy link
Author

Thanks for implementing this wish @jgabry!

@jgabry
Copy link
Member

jgabry commented Jul 25, 2017 via email

jgabry added a commit to stan-dev/rstanarm that referenced this issue Aug 1, 2017
need to move bayesplot to Depends in anticipation of the next bayesplot
release. This is needed so that bayesplot’s .onAttach function is
called when rstanarm is loaded. Otherwise the plotting theme will be
wrong unless bayesplot has already been manually loaded by the user
before loading rstanarm.

[ci skip]
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants