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

Allow default geom aesthetics to be set from theme #2749

Open
wants to merge 13 commits into
base: master
from

Conversation

Projects
None yet
4 participants
@dpseidel
Member

dpseidel commented Jul 11, 2018

Motivation

The goal of this PR is to allow certain default aesthetics to be set by the plot theme. This can be used to achieve better default behaviours (e.g. plotting white points by default, instead of black, when using theme_dark()) and to allow user created themes to more easily control default aesthetics. This PR addresses #2239.

Basic Approach

The basic approach is to alter each Geom's default_aes from something like:

# alpha must default to NA to respect RGBA color specification
default_aes = aes(colour = "black", size = 0.5, linetype = 1, alpha = NA)

to something more like:

default_aes = aes(
  colour = theme$geom$col,
  fill = theme$geom$fill,
  linetype = 1,
  alpha = NA
)

and then evaluate the default_aes mappings in an environment conscious of plot$theme. Since aes() quotes all arguments, I currently I do this by wrapping default_aes in expr() and evaluating it inside geom$use_defaults. This in turn gets called to build each layer and the guides. Some minor adjustments were also made to handle incomplete themes (I'm confident there is a better way, consider this a temporary patch) and to maintain update_geom_defaults() functionality.

@hadley, I think this is still slightly different from the algorithm we originally discussed. You originally indicated that we should evaluate the mappings within the plot environment and then call something like on.exit(set_geom_theme(NULL)), to reset to the default expression. I so far have been unable to make this work, and I think it may require our evaluation to happen earlier in the code. If this is still what you have in mind, I can reconsider and refactor the code. I agree it could be more streamline to evaluate these defaults upfront rather than at multiple points in the build, I simply have yet to make that work.

Next Steps

This PR is a work in progress. Currently I have only added theme elements colour and fill and only updated the default_aes for GeomPoint and GeomRect, eventually all geoms will need to be updated. This will mean making some decisions about how to handle variability across geom defaults. For example, looking just at colour: the default for geom_smooth(), geom_contour(), geom_density2d(), and geom_quantile() is all #3366FF and the rest of the geoms default to colour = "black" (or grey20 for boxplots and violins but this can and probably should be black also). It would make sense to set the default to black in theme_grey(), but that either means that geom_smooth() et al. should not be coded to inherit from colour from theme() ever, or these geoms will also be forced to default to black. I lean towards leaving these special defaults hardcoded in these cases as to not change expected behaviour. This may also influence exactly which elements should be set in themes (eg. size has more meaningful variability across geoms that we may want to preserve). A full list of geom defaults is here. From my analysis, I suspect we will want to allow theme() to set at least colour, fill, and alpha, potentially also shape, size and linetype. Font family and size should also be considered but likely should inherit from other text elements set by theme().

I have not yet updated documentation, added a NEWS bullet, or additional tests as all of this will be done further down the line. This branch does successfully build and pass tests. Running through existing package examples was very useful for finding side effects.

Visualizing the changes:

# theme_grey() is still our default and delivers expected defaults
ggplot(mpg, aes(displ, hwy)) + geom_point()

# custom themes can be added to change default colour and fill (other aesthetics may be added in the future)
my_theme <- theme(geom = element_geom(colour = "purple", fill = "darkblue"))
ggplot(mpg, aes(displ, hwy)) + geom_point() + my_theme

ggplot(mpg, aes(displ, hwy)) + geom_col() + my_theme

# theme dark now stands out automatically with lighter fill and colour defaults
ggplot(mtcars, aes(cyl, mpg)) + geom_col() + theme_dark() 

# theme had to be fed to guides to be able to handle discrete legends properly
ggplot(mpg, aes(displ, hwy, shape = drv)) + geom_point() + theme_dark()

# note that the mapping still overrides theme defaults
ggplot(mpg, aes(displ, hwy, colour = drv)) + geom_point() + my_theme

# and params still override all mapped and theme aesthetics
ggplot(mpg, aes(displ, hwy, colour = drv)) + geom_point(colour = "red") + my_theme

Created on 2018-07-10 by the reprex package (v0.2.0).

@clauswilke

This comment has been minimized.

Member

clauswilke commented Jul 11, 2018

I think this is going to be a great addition to ggplot2. One comment that dovetails with something @baptiste mentioned at some point in a related post: It's unlikely that there's going to be one choice of color that is going to work for all geoms. Usually, the way this is solved in design is via a small palette of choices. E.g., there could be a foreground color, a background/fill color, and a few accent colors. The geom defaults would then pick the appropriate choices from this palette.

As an example, themes in PowerPoint have 10 colors, background 1, background 2, text 1, text 2, and accent 1 through 6. For ggplot, where we often need to fill things, I'd propose to use something like background 1 & 2, text 1 & 2, accent 1 - 3, and fill 1 - 3. And if you wanted to make it even simpler, it could be just background, text, accent, and fill.

@dpseidel

This comment has been minimized.

Member

dpseidel commented Jul 11, 2018

Thanks @clauswilke - the palette idea would solve the variability issue I mentioned above. I just need to think critically about how to choose and document these palettes.

@baptiste

This comment has been minimized.

Contributor

baptiste commented Jul 11, 2018

Nice, thanks for doing this.

For reference, there could be some interesting ideas to pick from Gadfly

(notably: a number of foreground + background tints, but also the sort of automatic colour pairing that I mentioned in the issue (e.g deriving a nice palette by muting/complementing/... a base colour).

@hadley

This comment has been minimized.

Member

hadley commented Jul 11, 2018

I think we'll need at least colour.accent and fill.accent. I'm not sure how we'd define multiple accents (e.g. 1-3) to make it clear where they are used. The googlesheet should be helpful for pulling these together.

We might want to consider naming them colour, colour1 etc to avoid providing too many semantics.

@baptiste

This comment has been minimized.

Contributor

baptiste commented Jul 11, 2018

Alternatively, maybe vectors? colour[1:3]
It might require something like a length check to minimise confusion, but it would seem more extensible than a series of hard-coded variable names which don't add semantic value.

@hadley

This comment has been minimized.

Member

hadley commented Jul 11, 2018

Making it a vector suggests that there could be arbitrarily many of them. I don't know if that's any better.

@baptiste

This comment has been minimized.

Contributor

baptiste commented Jul 11, 2018

True – not entirely sure either if it's better.
But it does give more freedom for theme designers who might want a pool of N colours to pick from for whatever reason, theme_unicorn().

Mostly, I find names like colour1...colour2 quite non-descriptive and rather unhelpful (what if someone wants to generate such themes programmatically – e.g. with an app/add-in –, or extend their number, etc. -> immediately leads to things like assign()/get()/paste()).

@hadley

This comment has been minimized.

Member

hadley commented Jul 11, 2018

I don't think that's an axis of generalisation that will be helpful. This is about enabling built-in geoms to be themeable, so there is a fixed and known set of possible values.

@baptiste

This comment has been minimized.

Contributor

baptiste commented Jul 11, 2018

Not sure what's limiting this to built-in geoms – couldn't the ggunicorn package want to write

GeomUnicorn$default_aes = aes(horn.colour = theme$geom$colour[13])

?

@hadley

This comment has been minimized.

Member

hadley commented Jul 11, 2018

What I'm trying to say is that while we could make it more flexible, I don't think it is a good idea because it makes the implementation more complex for relatively little gain (since it requires both a custom theme and a custom geom, and shatters the independence of themes and geoms).

@baptiste

This comment has been minimized.

Contributor

baptiste commented Jul 11, 2018

Hmm, I guess we're not seeing it the same way – I'll leave it at what I wrote above, should others want to weigh in.
Personally I see no additional complexity in having the theme$geom element specify a vector of colours[1:n] rather than a fixed set of variables such as colour1...colour2. Built-in geoms as implemented in this PR would simply use theme$geom$colours[1], colours[2]... instead of theme$geom$colour1.
Unless I'm missing something this doesn't break the separation of geoms/themes any more than this whole proposal to inherit default values from the theme: it simply offers a more extensible way to do so beyond the built-in geoms, should the need arise in an extension package (and packages like ggthemes, etc. often do pair a theme, a scale, and a geom, in practice).

Edit: the only interaction I could see being a problem is if a user mixes a built-in theme that defines 3 colours with a geom that requires 4 in its default aes settings. But that doesn't seem technically unsurmountable – maybe set a hard limit of 5 and recycle shorter vectors.

dpseidel added some commits Jul 17, 2018

Adjust geom_density, geom_quantile, and geom_sf
geom_sf is now the only geom that does not allow aesthetic setting from themes due to a unique way of setting default aesthetics. Presumably should be adapted in a future commit.
@hadley

Basic approach looks good.

Let's iterate on how the default aesthetics look, and how they are evaluated.

R/geom-.r Outdated
@@ -107,11 +107,18 @@ Geom <- ggproto("Geom",
setup_data = function(data, params) data,
# Combine data with defaults and set aesthetics from parameters
use_defaults = function(self, data, params = list()) {
use_defaults = function(self, data, params = list(), theme) {

This comment has been minimized.

@hadley

hadley Aug 7, 2018

Member

I think the code related to evaluating aesthetics in the special theme environment should be pulled out into its own function so that we can think about it in isolation, and then test it.

@@ -200,7 +200,10 @@ GeomLogticks <- ggproto("GeomLogticks", Geom,
gTree(children = do.call("gList", ticks))
},
default_aes = aes(colour = "black", size = 0.5, linetype = 1, alpha = 1)
default_aes = expr(aes(

This comment has been minimized.

@hadley

hadley Aug 7, 2018

Member

I don't think you need the expr() wrapper here. aes() already captures the unevaluated aesthetics?

This comment has been minimized.

@dpseidel

dpseidel Aug 8, 2018

Member

It does, as quosures, so iirc, without expr() I ended up needing to do some quosure hacking -- to set the environment to one that understood theme. As I refactor the theme evaluation into it's own function as you suggest above, I'll aim to eliminate this as well.

@@ -200,7 +200,10 @@ GeomLogticks <- ggproto("GeomLogticks", Geom,
gTree(children = do.call("gList", ticks))
},
default_aes = aes(colour = "black", size = 0.5, linetype = 1, alpha = 1)
default_aes = expr(aes(
colour = theme$geom$colour, size = 0.5, linetype = 1,

This comment has been minimized.

@hadley

hadley Aug 7, 2018

Member

I think theme$geom$colour is a bit long and a bit magical. I think it would be better to have a function like foo("colour"), but I don't know what we'd call it.

default_aes = aes(colour = "black", size = 0.5, linetype = 1, alpha = NA),
default_aes = expr(aes(
colour = theme$geom$colour,
size = 0.5, linetype = 1,

This comment has been minimized.

@hadley

hadley Aug 7, 2018

Member

As you re-write these, can you put each aesthetic on its own line?

@@ -13,7 +13,11 @@
#' @rdname update_defaults
update_geom_defaults <- function(geom, new) {
g <- check_subclass(geom, "Geom", env = parent.frame())
old <- g$default_aes
env <- new.env()

This comment has been minimized.

@hadley

hadley Aug 7, 2018

Member

If geoms are themeable, I think we can deprecate this function, so we should just leave as is and in the documentation mark it as internal, and point people towards the new theme system.

This comment has been minimized.

@dpseidel

dpseidel Aug 8, 2018

Member

If we deprecate this, we will presumably need to make all aesthetics themeable so as to not lose the functionality?

This comment has been minimized.

@hadley

hadley Aug 10, 2018

Member

Oh yeah. In that case we should pull this out into a function that can be shared with eval_defaults()

R/layer.r Outdated
if (empty(data)) return(data)
self$geom$use_defaults(data, self$aes_params)
# Combine aesthetics, defaults, & params
self$geom$use_defaults(data, self$aes_params, plot$theme)

This comment has been minimized.

@hadley

hadley Aug 7, 2018

Member

When you rewrite these calls with many unnamed arguments, can you please rewrite to add names? (i.e. here use theme = plot$theme)

@@ -178,11 +178,11 @@ GeomSf <- ggproto("GeomSf", Geom,
default_aesthetics <- function(type) {
if (type == "point") {
GeomPoint$default_aes
aes(shape = 19, colour = "black", size = 1.5, fill = NA, alpha = NA, stroke = 0.5)

This comment has been minimized.

@hadley

hadley Aug 7, 2018

Member

Why can we no longer inherit from point?

This comment has been minimized.

@dpseidel

dpseidel Aug 8, 2018

Member

Because with the expr() wrapper the default_aes no longer played nice with defaults() function or utils::modifyList() which is used by sf. I expect this can/will be reverted with this round of changes.

This comment has been minimized.

@dpseidel

dpseidel Aug 9, 2018

Member

Actually now that I'm looking at this again, geom_sf can't currently inherit any "themeable" aesthetics because this function (and thus these aesthetics) get evaluated in draw_geom step of layer.r (rather than earlier in the get_defaults step as do all other geoms), so it currently passes an unevaluated expression to grDevices::col2rgb() which predictably gives an error . So this needs some refactoring to be "themeable" - either by evaluating aesthetics at the proper step (rather than passing NULL values as geom_sf does now) or by pulling theme into draw_geom so that the sf aesthetics can be evaluated in the context of the plots theme. It would seem to me that the former is a preferable solution but I expect that it was written this way for a reason that will become obvious as I dig in. I'm fixing your other concerns now and will work on sf in a future commit.

#' @param colour.accent2,color.accent2 accent colour 2,
#' typically a bright colour used for geom_smooth et al.
#' @param fill.accent accent fill colour, typically a darker version of fill
#' @param alpha colour/fill transparency, between 0 & 1.

This comment has been minimized.

@hadley

hadley Aug 7, 2018

Member

Do we need alpha as a default? People could always set individually with alpha()

This comment has been minimized.

@dpseidel

dpseidel Aug 8, 2018

Member

Presumably not. Currently alpha has to be NA by default to respect RGBA colour specification

#' @rdname element
element_geom <- function(fill = NULL, fill.accent = NULL,
colour = NULL, color = NULL,
colour.accent1 = NULL, color.accent1 = NULL,

This comment has been minimized.

@hadley

hadley Aug 7, 2018

Member

What if we used fill and fill_1?

And what if we ditched colour + color in favo(u)r of col?

This comment has been minimized.

@clauswilke

clauswilke Aug 8, 2018

Member

With the aesthetics renaming code that I wrote, we should be able to get away with defining just one spelling of colour (probably the British one). The intent was to internally rename everything to that spelling. It probably already works as is, but worst-case scenario a few more renaming calls would have to be added in some places.

This comment has been minimized.

@clauswilke

clauswilke Aug 8, 2018

Member

I.e., take the argument names of element_geom() and run through this function:

ggplot2/R/aes.r

Line 155 in 4d2ca99

standardise_aes_names <- function(x) {

This comment has been minimized.

@clauswilke

clauswilke Aug 8, 2018

Member

However, this would require a ... argument instead of listing everything explicitly, so maybe not the right approach. In any case, I’d argue against col just because in the rest of ggplot2 col is not used. And I think we also should talk about whether it’s colour.accent1 or colour_accent1. We’re not entirely consistent with points vs underscores, and it would be good to pick a scheme and at least be consistent going forward.

dpseidel added some commits Aug 8, 2018

merge master into theme_geom
Merge remote-tracking branch 'upstream/master' into theme_geom

# Conflicts:
#	R/theme.r
#	man/element.Rd
#	man/theme.Rd

@dpseidel dpseidel force-pushed the dpseidel:theme_geom branch from 8cc0c37 to 23fdf39 Aug 9, 2018

@dpseidel

This comment has been minimized.

Member

dpseidel commented Aug 9, 2018

So I've implemented most of your suggested changes at least enough so that we can iterate on them. A couple notes:

  1. I've removed the expr() wrapper which makes a lot of sense but has some downstream effects, notably, since aes() quotes its arguments and given environments are ignored when evaluating quosures (as they have their own)-- I'm now evaluating default_aes() by passing the plot theme as data to eval_tidy(). This works fine with the slightly magical and long theme$geom$col expression or with the less magical (?) but necessarily (AFAICT) 2 argument function theme_aes("col", theme) that I have implemented for GeomPoint as an example.

    ggplot2/R/geom-point.r

    Lines 108 to 118 in 23fdf39

    GeomPoint <- ggproto("GeomPoint", Geom,
    required_aes = c("x", "y"),
    non_missing_aes = c("size", "shape", "colour"),
    default_aes = aes(
    shape = 19,
    colour = theme_aes("col", theme),
    size = 1.5,
    fill = NA,
    alpha = NA,
    stroke = 0.5
    ),

    If I want to be able to pass an environment I have to hack the quosures before evaluation which seems really unconventional or consider adapting the aes() or new_aesthetics() functions to specially handle themeable elements which seems like a potential dangerzone for breaking things I don't want to break. Is there a different approach I am missing? Do you still prefer the function or some variant of it?
  2. So far I have left the update_geom_defaults() function and geom_sf() as is (see response to your earlier comment), but those will be fixed in a future commit.
  3. I updated to col, col_1, etc. but that discussion is more than warranted and I'm happy to change things again.
  4. I'll begin adding tests in the next iteration.
@clauswilke

This comment has been minimized.

Member

clauswilke commented Aug 10, 2018

How about implementing element_geom() along the following lines?

element_geom <- function(fill = NULL,
                         fill_1 = NULL,
                         colour = NULL,
                         colour_1 = NULL,
                         colour_2 = NULL,
                         ...,
                         inherit.blank = FALSE) {
  extra_aes <- ggplot2:::rename_aes(list(...))
  
  aes_list <- modifyList(
    list(
      fill = fill, fill_1 = fill_1, colour = colour,
      colour_1 = colour_1, colour_2 = colour_2,
      inherit.blank = inherit.blank
    ),
    extra_aes,
    keep.null = TRUE
  )
  
  structure(
    aes_list,
    class = c("element_geom", "element")
  )
}

element_geom()
#> List of 6
#>  $ fill         : NULL
#>  $ fill_1       : NULL
#>  $ colour       : NULL
#>  $ colour_1     : NULL
#>  $ colour_2     : NULL
#>  $ inherit.blank: logi FALSE
#>  - attr(*, "class")= chr [1:2] "element_geom" "element"

element_geom(fill = "red", colour = "blue")
#> List of 6
#>  $ fill         : chr "red"
#>  $ fill_1       : NULL
#>  $ colour       : chr "blue"
#>  $ colour_1     : NULL
#>  $ colour_2     : NULL
#>  $ inherit.blank: logi FALSE
#>  - attr(*, "class")= chr [1:2] "element_geom" "element"

element_geom(fill = "red", color = "blue", color_1 = "black")
#> List of 6
#>  $ fill         : chr "red"
#>  $ fill_1       : NULL
#>  $ colour       : chr "blue"
#>  $ colour_1     : chr "black"
#>  $ colour_2     : NULL
#>  $ inherit.blank: logi FALSE
#>  - attr(*, "class")= chr [1:2] "element_geom" "element"

Created on 2018-08-09 by the reprex package (v0.2.0).

R/geom-.r Outdated
eval_defaults = function(self, theme) {
if (length(theme) == 0) theme <- theme_grey()
lapply(self$default_aes, rlang::eval_tidy, data = list(theme = theme))

This comment has been minimized.

@hadley

hadley Aug 10, 2018

Member

This seems like the right approach to me, but I think I'd embed I do:

from_theme <- function(name) {
  # plus error handling
  theme$geom[[name]]
}

Then data = list(from_theme = from_theme)

@dpseidel

This comment has been minimized.

Member

dpseidel commented Sep 7, 2018

Sorry for the long delay on my end, getting back to this now. I have just embedded and implemented from_theme() as suggested by @hadley and refactored element_geom() as suggested by @clauswilke. Unfortunately, with vdiffr now running properly, this PR will fail on multiple unrelated visual tests because of the way vdiffr adds it's own theme_test() to all plots without a specified theme. vdiffr::theme_test() does not recognize/specify the new element_geom() and thus serves colour = NULL to scales::alpha() which returns a subscript out of bounds error. All non-visual tests pass. I need to add new tests, visual especially, but won't be able to do this without a workaround or solution to the theme_test() issue. I am still working to refactor sf to handle themeable aesthetics and update documentation, examples, and tests.

@clauswilke

This comment has been minimized.

Member

clauswilke commented Sep 7, 2018

Dana, I think the solution to your vdiffr problem is in lionel-/vdiffr#34, "Move theme_test to ggplot2".

Also, I assume that the theme changes you've been working on are out of scope for the upcoming 3.0.1 release, because they may break existing 3rd-party code. Therefore, the right order of things is probably:

  • Move theme_test() to ggplot2 for 3.0.1
  • Change vdiffr once ggplot2 3.0.1 is released (or make it depend on the development version)
  • After 3.0.1, merge this PR into master

I have some theme changes pending that also don't fit into 3.0.1 (#2784), so maybe the release thereafter could do a major theme overhaul. I also had some thoughts about handling missing default elements better, but I don't remember whether I had integrated that already in my PR or not (I think not, though). There's also the idea of adding scales to the theme (#2691), and that could also be tackled for a major theme overhaul.

@dpseidel

This comment has been minimized.

Member

dpseidel commented Sep 7, 2018

Oh, interesting. Yes I suspected we would need to make a change in vdiffr but ggplot2 actually already has it's own theme_test()

theme_test <- function(base_size = 11, base_family = "",

It's just not yet used by vdiffr, so presumably, I can open a PR there without waiting for the bug release.

Ultimately though, I think unless we make vdiffr depend on the development version, there will be a stage before release where the visual tests break given the changes to theme_test() in this PR, even once vdiffr is using ggplot2::theme_test() just because this PR is building a whole new element tree.

Regardless, as you say, this is most definitely outside the intended scope of 3.0.1, so there is no big rush I am just trying to keep chipping away at this. I agree between open PRs and related issues, there is plenty to work on for a general theme overhaul and I would be happy to help.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment