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

theme(x, y) versus theme(x) + theme(y) #3039

Closed
ptoche opened this issue Dec 15, 2018 · 10 comments · Fixed by microly/ggplot2#2
Closed

theme(x, y) versus theme(x) + theme(y) #3039

ptoche opened this issue Dec 15, 2018 · 10 comments · Fixed by microly/ggplot2#2
Labels
bug themes 💃

Comments

@ptoche
Copy link

@ptoche ptoche commented Dec 15, 2018

I had always assumed that the following were equivalent. They are not. Is it a feature of theming that all the options must be set in theme() in one pass?

If so, may I suggest adding an example like below to: https://ggplot2.tidyverse.org/reference/theme.html

library(reprex)

library(ggplot2)
ggplot(mtcars, aes(wt, mpg)) +
    geom_point() +
    theme_void() + 
    theme(axis.line.x = element_line(color = "blue"),
          axis.ticks.x = element_line(color = "red"),
          axis.ticks.length = unit(1, "cm")) 

Created on 2018-12-15 by the reprex package (v0.2.1)

library(reprex)
library(ggplot2)
ggplot(mtcars, aes(wt, mpg)) +
    geom_point() + 
    theme_void() + 
    theme(axis.line.x = element_line(color = "blue")) +
    theme(axis.ticks.x = element_line(color = "red")) +
    theme(axis.ticks.length = unit(1, "cm")) 

Created on 2018-12-15 by the reprex package (v0.2.1)

@ptoche
Copy link
Author

@ptoche ptoche commented Dec 15, 2018

Perhaps the same issue: To display the axis line, I need to set axis.ticks.length. Change unit(1, "cm") to unit(0, "cm") or drop that last line altogether and the axis line disappears.

ggplot(mtcars, aes(wt, mpg)) +
    geom_point() +
    theme_void() + 
    theme(axis.line.x = element_line(color = "blue"),
           axis.ticks.length = unit(1, "cm")) 

@clauswilke
Copy link
Member

@clauswilke clauswilke commented Dec 15, 2018

This is a bug with theme_void(). It declares itself to be a complete theme but misses elements. This keeps coming up in various ways and the correct way to fix it is to define all theme elements in theme_void(). See e.g. #2723.

library(ggplot2)
p1 <- ggplot(mtcars, aes(wt, mpg)) +
  geom_point() +
  theme_classic() + 
  theme(axis.line.x = element_line(color = "blue"),
        axis.ticks.x = element_line(color = "red"),
        axis.ticks.length = unit(1, "cm")) 

p2 <- ggplot(mtcars, aes(wt, mpg)) +
  geom_point() + 
  theme_classic() + 
  theme(axis.line.x = element_line(color = "blue")) +
  theme(axis.ticks.x = element_line(color = "red")) +
  theme(axis.ticks.length = unit(1, "cm")) 

cowplot::plot_grid(p1, p2)

Created on 2018-12-15 by the reprex package (v0.2.1)

@clauswilke
Copy link
Member

@clauswilke clauswilke commented Dec 15, 2018

Actually, while theme_void() is still missing theme elements that it should define, maybe the right place to point the finger to here is this issue: #2724

In any case, it's a bug.

@clauswilke
Copy link
Member

@clauswilke clauswilke commented Dec 15, 2018

Third time is a charm. I think I've found the real culprit: theme_void() sets line = element_blank(), and that means if downstream elements try to inherit from line they don't get reasonable defaults. It's actually surprising that the first example works, and I'm still not sure why. In any case, if line is defined appropriately things work as one would expect:

library(ggplot2)
ggplot(mtcars, aes(wt, mpg)) +
  geom_point() + 
  theme_void() +
  theme(line = element_line(size = 0.5, linetype = 1, lineend = "butt", color = NA)) +
  theme(axis.line.x = element_line(color = "blue")) +
  theme(axis.ticks.x = element_line(color = "red")) +
  theme(axis.ticks.length = unit(1, "cm")) 

Created on 2018-12-15 by the reprex package (v0.2.1)

This still means theme_void() needs to be rethought. I think it should define a standard line element and then set the downstream elements (axis.line(), axis.ticks(), etc.) to element_blank().

@ptoche
Copy link
Author

@ptoche ptoche commented Dec 15, 2018

Thanks for the feedback and sleuthing Claus!

I suspect theme_minimal() is also incomplete? Same apparent problem as with theme_void().

No problem that I can see with theme_bw(), theme_gray(), theme_classic().

Not sure about theme_light() and theme_dark(), which I don't use. This is the output:

library(reprex)
library(ggplot2)
ggplot(mtcars, aes(wt, mpg)) +
    geom_point() + 
    theme_light() + 
    theme(axis.line.x = element_line(color = "blue")) +
    theme(axis.ticks.x = element_line(color = "red")) +
    theme(axis.ticks.length = unit(1, "cm"))

Created on 2018-12-16 by the reprex package (v0.2.1)

@clauswilke
Copy link
Member

@clauswilke clauswilke commented Dec 15, 2018

There seem to be multiple issues at play here. There clearly is a difference between theme(x, y) and theme(x) + theme(y), and there shouldn't be.

library(ggplot2)
p1 <- ggplot(mtcars, aes(wt, mpg)) +
  geom_point() + 
  theme_light() + 
  theme(axis.line.x = element_line(color = "blue")) +
  theme(axis.ticks.x = element_line(color = "red")) +
  theme(axis.ticks.length = unit(1, "cm"))

p2 <- ggplot(mtcars, aes(wt, mpg)) +
  geom_point() + 
  theme_light() + 
  theme(axis.line.x = element_line(color = "blue"),
        axis.ticks.x = element_line(color = "red"),
        axis.ticks.length = unit(1, "cm"))

cowplot::plot_grid(p1, p2)

Created on 2018-12-15 by the reprex package (v0.2.1)

@clauswilke
Copy link
Member

@clauswilke clauswilke commented Dec 15, 2018

The fundamental problem here is that adding themes to the plot object doesn't behave the same as adding themes to each other and seems buggy. Specifically, adding additional theme elements seems to convert parts of an incomplete theme into parts of a complete theme in the plot building process.

library(ggplot2)

t1 <- theme_light() + 
  theme(axis.line.x = element_line(color = "blue")) +
  theme(axis.ticks.x = element_line(color = "red"))

t2 <- theme_light() + 
  theme(axis.line.x = element_line(color = "blue"),
        axis.ticks.x = element_line(color = "red"))

identical(t1, t2)
#> [1] TRUE

p <- ggplot(mtcars, aes(wt, mpg)) + geom_point()
p1 <- p + theme_light() + 
  theme(axis.line.x = element_line(color = "blue")) +
  theme(axis.ticks.x = element_line(color = "red"))

p2 <- p + theme_light() +
  theme(axis.line.x = element_line(color = "blue"),
        axis.ticks.x = element_line(color = "red"))

identical(p1$theme, p2$theme)
#> [1] FALSE

p1$theme$axis.line.x$inherit.blank # incorrect
#> [1] TRUE
p2$theme$axis.line.x$inherit.blank # correct
#> [1] FALSE

p1

p1$theme$axis.line.x$inherit.blank <- FALSE
p1

Created on 2018-12-15 by the reprex package (v0.2.1)

@clauswilke
Copy link
Member

@clauswilke clauswilke commented Dec 15, 2018

I've tracked it down. When themes are added to the plot the function that is used is update_theme():

ggplot_add.theme <- function(object, plot, object_name) {
plot$theme <- update_theme(plot$theme, object)
plot

But when two themes are added together directly, the function is add_theme():

if (is.theme(e1)) add_theme(e1, e2, e2name)

The two functions have different logic, because update_theme() pulls in information from the default theme if otherwise missing:

ggplot2/R/theme.r

Lines 470 to 493 in 442108d

update_theme <- function(oldtheme, newtheme) {
# If the newtheme is a complete one, don't bother searching
# the default theme -- just replace everything with newtheme
if (is_theme_complete(newtheme))
return(newtheme)
# These are elements in newtheme that aren't already set in oldtheme.
# They will be pulled from the default theme.
newitems <- !names(newtheme) %in% names(oldtheme)
newitem_names <- names(newtheme)[newitems]
oldtheme[newitem_names] <- theme_get()[newitem_names]
# Update the theme elements with the things from newtheme
# Turn the 'theme' list into a proper theme object first, and preserve
# the 'complete' attribute. It's possible that oldtheme is an empty
# list, and in that case, set complete to FALSE.
old.validate <- isTRUE(attr(oldtheme, "validate"))
new.validate <- isTRUE(attr(newtheme, "validate"))
oldtheme <- do.call(theme, c(oldtheme,
complete = isTRUE(attr(oldtheme, "complete")),
validate = old.validate & new.validate))
oldtheme + newtheme
}

I think the right logic would be to first add all the theme parts together with add_theme() during plot construction, and only fill in the missing pieces once the plot is built. This fits logically with the changes discussed in PR #2784.

@ptoche
Copy link
Author

@ptoche ptoche commented Jan 11, 2019

I think we agree this is a bug. How can I add a "bug" label to this issue? Do I need special rights or have I missed the icon on which to click? Currently it says "Labels: 'None yet'". Thanks.

@lock
Copy link

@lock lock bot commented Apr 25, 2020

This old issue has been automatically locked. If you believe you have found a related problem, please file a new issue (with reprex) and link to this issue. https://reprex.tidyverse.org/

@lock lock bot locked and limited conversation to collaborators Apr 25, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
bug themes 💃
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants