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

setting global theme: costs vs. benefits #117

Closed
grasshoppermouse opened this issue Sep 24, 2017 · 17 comments
Closed

setting global theme: costs vs. benefits #117

grasshoppermouse opened this issue Sep 24, 2017 · 17 comments

Comments

@grasshoppermouse
Copy link

It took me a few minutes to discover why all the figures in our paper changed. Answer: one of the co-authors added some code that used bayesplot. It took another few minutes of googling to figure out how to fix that. Not a huge deal, but it's nice when loading and using a package that there are as few unintended side effects as possible. On the other hand, there are benefits if you are only using bayesplot and/or like the theme. For me, the (small) costs outweigh the benefits.

Thoughts on the cost/benefit tradeoff of setting the global ggplot theme?

@jgabry
Copy link
Member

jgabry commented Sep 24, 2017

Thanks for the comment. The bayesplot package didn't used to set the ggplot theme when it was loaded. It used to just hardcode a theme for each plot without affecting the rest of ggplot2, but that was actually changed in a recent release in order to make it easier to change the theme of bayesplot plots (#87). That is, ggplot2::theme_set didn't used to affect bayesplot plots but with this change you can now use it just like you would with regular ggplot2.

You can see from the discussion at #87 that I was hesitant to do this because it changes the ggplot theme for all plots when bayesplot is loaded. But I was persuaded that since there's a precedent for this behavior in other plotting packages, the benefits outweighed the costs. That said, I can totally understand that for some people the costs outweigh the benefits.

@grasshoppermouse
Copy link
Author

OK, thanks. I did search for a previous issue but missed that one. I also hate that cowplot changes the global theme too, as apparently do some other folks:

https://stackoverflow.com/questions/41096293/cowplot-made-ggplot2-theme-disappear-how-to-see-current-ggplot2-theme-and-res

So now I avoid loading that package if at all possible. I guess a solution is to use bayesplot::

In any case, thanks for the package!

@jgabry
Copy link
Member

jgabry commented Sep 24, 2017 via email

@jgabry
Copy link
Member

jgabry commented Sep 24, 2017 via email

@tjmahr
Copy link
Collaborator

tjmahr commented Sep 24, 2017 via email

@tjmahr
Copy link
Collaborator

tjmahr commented Sep 26, 2017

Looking back at the issue by @gavinsimpson, #87...

We don't want to be impolite and mess with the global settings. I went four years of using ggplot2 without ever having to use theme_set(), but I had to learn it in order to use bayesplot. But we want to spare users the tedium of having to overwrite theme on every bayesplot theme.

What if we do theme_set_bayesplot()? On load, it's the bayesplot default. But the user can override it with theme_set_bayesplot()?

@gavinsimpson
Copy link

@tjmahr I think that is what I had suggested in #87 (although I don't follow why this is theme_set_bayesplot() and not theme_set(theme_bayesplot())?). A popular package that uses .onAttach() to set the theme globally in the workspace is cowplot. It used to frustrate me as a user that cowplot changed the default as I was really only interested in its layout functions. Given the triviality of changing the theme back to what I want (I use theme_bw() in general), and the fact that users of bayesplot are doing so for the plots (which the developers have given nice thought to) I don't see it as too much of an intrusion to set the theme globally.

Importantly, however, (re)setting the theme should be done via standard functions, e.g. theme_set(theme_foo()).

Whether you want to automatically set this depends on who you want to frustrate most; existing users who now find all their plots change if you don't automatically set the theme to the bayesplot default, or new users that never saw the hard-coded bayesplot theme and don't know why all their ggplot plots now look different.

I'm happy either way.

@tjmahr
Copy link
Collaborator

tjmahr commented Sep 29, 2017

I am proposing that bayesplot have its own theme_set and theme_get functions.

Basic idea:

  • In bayesplot plotting functions, use theme_get_bayesplot() to get the current theme for bayesplot functions.
  • On package load, call theme_set_bayesplot(theme_bayesplot()) to set the theme for bayesplot functions to the default. The user's current global ggplot2 theme is unchanged.
  • If the user wants to override the theme for bayesplot plots, they can send a theme to theme_set_bayesplot(). The user doesn't have to re-theme every plot from bayesplot.

This solves both issues:

  1. We don't mess with the global plotting theme on package load.
  2. Users can set a theme to override the default bayesplot look.

@gavinsimpson
Copy link

@tjmahr Hmmm. Doesn't that make it harder to discover what's going on and how to turn it off?

But intriguing suggestion; are you proposing to have all plots in bayesplot have + bayesplot_theme_obj (or some such named theme object), with that theme object controlled via theme_set_bayesplot().

That's certainly different and will likely satisfy everyone. Interested to see how this progresses.

@tjmahr
Copy link
Collaborator

tjmahr commented Sep 29, 2017 via email

@jgabry
Copy link
Member

jgabry commented Mar 19, 2018

This issue is a bit old now but I do think this is something we should revisit. Over at stan-dev/rstanarm#257 I proposed something similar to @tjmahr's proposal here. I'm still not sure what the best thing to do is, as all of the options seem to have downsides, but let's get this conversation started again so we can settle on something soon.

@malcolmbarrett
Copy link
Contributor

Oh yes, what @tjmahr is describing is similar to what I imagined based on @jgabry's comment here stan-dev/rstanarm#258. I also shared some potential code for it there. I think it's any interesting solution and, like I mentioned over there, I think it would be easy to make it play nice with existing changes to the default theme set by a user by comparing whatever the theme is to theme_gray() for any differences (e.g. could have bayes_theme_set() check if theme_get() is different from theme_gray() and if so just use the user's default but otherwise use the default bayesplot theme).

I do, though, agree that all of these ways of dealing with it have their downsides. There's also something to be said about simply including a theme and not having it be defaulted anywhere, although it's probably late in the game for you all for something like that.

Anyway, let me know if I can help.

@jgabry
Copy link
Member

jgabry commented Apr 28, 2018

@malcolmbarrett If you’re interested in making a PR for this we’d be happy to have it. @tjmahr or I will definitely get to this eventually regardless, but you put a lot of work into that rstanarm PR that’s obviated by this change to bayesplot and I wanted to give you the opportunity to contribute a PR that’s likely to get merged! No worries if you don’t have the time, but if you’re up for it that would be great!

@malcolmbarrett
Copy link
Contributor

Yes, I can give this a shot next week. The skeleton I posted looks like a good approach? I would also really like to embed a little hierarchy, e.g. if the user has set bayes_theme_set() to something other than the default, use that, then check against ggplot2::theme_get() to see if different than theme_grey(), otherwise use the default bayes theme. That way the user can just use ggplot2::theme_set() without having to think about it (or learn a new approach) but someone who is really just focused on bayesplot can specify both (or none, just using defaults) if desired

@tjmahr
Copy link
Collaborator

tjmahr commented Apr 29, 2018 via email

@malcolmbarrett
Copy link
Contributor

Yes, my thinking is that it would only change to whatevrer is in theme_get() if the user has done theme_set(), which is to say that if it's theme_grey, it would not override the bayesplot theme.

@jgabry
Copy link
Member

jgabry commented Apr 29, 2018

I agree, I think having bayesplot_theme_[sg]et (and I guess also bayesplot_theme_update) would be great.

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

No branches or pull requests

5 participants