-
-
Notifications
You must be signed in to change notification settings - Fork 84
Make the bayesplot default theme optional? #87
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
Comments
Hi Gavin, thanks for the suggestion! Will this proposed method result in
changing the plotting theme for any ggplot2 plot created by the user
regardless of whether or not it is from bayesplot? That was my original
motivation for not doing it this way, i.e. to try to keep bayesplot stuff
only affecting bayesplot stuff. If I'm wrong about that or if I'm right but
users don't mind that behavior then I'd be open to switching to what you
propose.
…On Sun, Apr 23, 2017 at 12:52 PM Gavin Simpson ***@***.***> wrote:
At the moment, from what I can tell having only had a brief look at the
code, you are hard coding the*bayesplot* 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.
—
You are receiving this because you are subscribed to this thread.
Reply to this email directly, view it on GitHub
<#87>, or mute the thread
<https://github.com/notifications/unsubscribe-auth/AHb4Q70_j22znQrrYdtNMInf7ogGPgrkks5ry4HggaJpZM4NFdyE>
.
|
@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 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 If you were to flag the issue, as cowplot does in documentation and in the |
Ok I think I'm persuaded. Shouldn't be hard to change so I'll try to get it
in for the next release. Thanks for following up.
…On Mon, Apr 24, 2017 at 2:21 PM Gavin Simpson ***@***.***> wrote:
@jgabry <https://github.com/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.
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#87 (comment)>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/AHb4Q5SM5QElqLOVeBsww1RhqiYpMMV8ks5rzOgWgaJpZM4NFdyE>
.
|
PR #95 merged so closing this |
Thanks for implementing this wish @jgabry! |
Thanks for suggesting it!
…On Tue, Jul 25, 2017 at 6:15 PM, Gavin Simpson ***@***.***> wrote:
Thanks for implementing this wish @jgabry <https://github.com/jgabry>!
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#87 (comment)>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/AHb4Q3P9_LGFSA_qQP8EwjmnBGX_G1-Zks5sRmj1gaJpZM4NFdyE>
.
|
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]
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: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.The text was updated successfully, but these errors were encountered: