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

Enable User Defined Theme Elements #2540

Closed
nhamilton1980 opened this issue Apr 27, 2018 · 32 comments
Closed

Enable User Defined Theme Elements #2540

nhamilton1980 opened this issue Apr 27, 2018 · 32 comments
Labels
feature internals 🔎 themes 💃

Comments

@nhamilton1980
Copy link

@nhamilton1980 nhamilton1980 commented Apr 27, 2018

This issue is at the suggestion of Hadley, based on the following:

#2305

I am the author of the ggtern package, which requires a whole bunch (50+) of new theme elements outside of the standard ggplot2 themes defined in the non-exported .element_tree object.

In this issue I will speak of my experience with ggtern, however, it is possible that any extension author may face the same problem.

The complexity of ggtern (and its sensitivity to changes within ggplot2) can be HUGELY reduced if there was a mechanism for extension authors such as myself to be able to create their own theme elements, and add them to the .element_tree. The solution I had proposed within the above link (via use of R6 class) had resolved this, with no impinging effect or risk to existing ggplot2 functionality.

Because I cannot add new theme elements, this means that I need to duplicate the .element_tree within the ggtern package, and add my new theme elements / hierarchies. This in turn means that many of the plot construction routines also needs to be held as a duplicate inside the ggtern library, leading to fragilities and vulnerabilities to changes within base ggplot2, which happens almost every year. I am trying to remove these fragilities.

To me it seems very logical for extension authors to be able to create their own theme elements. At the moment there is no way to do this to my knowledge other than the method used in ggtern, which is far from ideal. The theme element tree offered by ggplot2 is quite substantial, but by no means is it exhaustive, as has been demonstrated in ggtern, requiring dozens more for it to function properly.

@nhamilton1980 nhamilton1980 changed the title Enable User Defined Theme Elelmets Enable User Defined Theme Elements Apr 28, 2018
@clauswilke
Copy link
Member

@clauswilke clauswilke commented May 9, 2018

I have reviewed your pull request. If I understand correctly what you need for ggtern, your proposed solution seems way overengineered. In particular, adding a whole new class model (R6) to ggplot2 doesn't seem warranted. I'd also like to point out that your code doesn't follow the tidyverse style guide (http://style.tidyverse.org/), in particular with respect to spaces around if, else, {, and after commas.

If all you need is the ability to add new theme elements, that functionality can be achieved with one new function, add_element(). The only issue is that direct assignment to .element_tree is not possible, so it'd have to be stuck into an environment, like so:

element_env <- new.env(parent = emptyenv())
element_env$element_tree <- list()

add_element <- function(key, element_def) {
  if (is.null(element_env$element_tree[[key]])) {
    element_env$element_tree <- c(
      element_env$element_tree,
      setNames(list(element_def), key)
    )
  } else {
    warning("trying to redefine existing element.")
  }
}

Now, we can do:

add_element("text", ggplot2:::el_def("element_text"))
add_element("title", ggplot2:::el_def("element_text", "text"))
add_element("text", ggplot2:::el_def("element_line"))
# Warning message:
# In add_element("text", ggplot2:::el_def("element_line")) :
#   trying to redefine existing element.

element_env$element_tree[["text"]]
# $class
# [1] "element_text"
# 
# $inherit
# NULL
# 
# $description
# NULL

Not sure why you would need all the other features you make available, such as overwriting theme elements on the fly. Would you ever have the situation where a theme element's definition would change after the initial package setup?

@hadley
Copy link
Member

@hadley hadley commented May 9, 2018

Is this the same as #1730?

@nhamilton1980

This comment has been minimized.

@nhamilton1980
Copy link
Author

@nhamilton1980 nhamilton1980 commented May 9, 2018

@clauswilke
Copy link
Member

@clauswilke clauswilke commented May 10, 2018

Nicholas,

I wanted to make sure the base set of theme elements couldn’t get messed around with, whist still providing the ability for users to add new elements.

That would be the case with my proposed approach, since theme elements can be added but only if the name isn't taken yet. Is this sufficient for you, or do you need additional features?

@nhamilton1980
Copy link
Author

@nhamilton1980 nhamilton1980 commented May 10, 2018

@clauswilke
Copy link
Member

@clauswilke clauswilke commented May 10, 2018

I think a solution along those lines can be found. However, unfortunately I think it's too late for 2.3.0. I think a reasonable goal would be to try to add the extension functionality ggtern needs into the next version after 2.3.0, so that bug reports such as nicholasehamilton/ggtern#16 and wilkelab/cowplot#88 can be closed.

@nhamilton1980
Copy link
Author

@nhamilton1980 nhamilton1980 commented May 11, 2018

Fantastic. This will be extremely beneficial to me.

@clauswilke
Copy link
Member

@clauswilke clauswilke commented May 22, 2018

See also #2649 for a different problem that requires a similar technical fix: a currently hard-coded list needs to become user-configurable.

@clauswilke
Copy link
Member

@clauswilke clauswilke commented Jul 7, 2018

@hadley I'm happy to take this one on. One question: We'll need an environment to store the element tree. We already have an environment to store the current theme:

theme_env <- new.env(parent = emptyenv())
theme_env$current <- theme_gray()

We'll also need an environment to address issue #2649. Do you want all these environments to be separate with their own names, or would it make sense to consolidate and have one environment (e.g. ggplot_global or ggplot_env) in which we store all these global settings. My own preference would be to consolidate.

@hadley
Copy link
Member

@hadley hadley commented Jul 7, 2018

I think we should consolidate. However, we'll need to carefully consider the implications of making these user modifiable — I can imagine this could lead to much confusion if loading a package can affect what aesthetics ggplot2 knows about.

@clauswilke
Copy link
Member

@clauswilke clauswilke commented Jul 7, 2018

@hadley Yes, I want to start with theme elements, where I think the risk is lower and the cost of not doing it is higher. The current situation with ggtern is not tenable. It is very popular, it breaks every time ggplot2 is updated, and there are also unexpected side effects with other packages in the ggplot universe (e.g. here: nicholasehamilton/ggtern#16). I'll comment on aesthetics in #2649.

@clauswilke
Copy link
Member

@clauswilke clauswilke commented Jul 11, 2018

@hadley I tried to move the element tree into the theme itself, as you suggested here. However, I ran into an issue that I can't resolve without making things rather ugly: How does the theme() function know about the element tree it should validate against?

Specifically, consider the following example. Let's say I have defined a theme theme_custom() that defines an element tree with a nonstandard element element_box(). Now I want to modify that theme by changing the color of a box:

theme_custom() + theme(some.box = element_box(color = "red)) 

I don't see how the theme() call in this example can have access to the element tree defined in theme_custom(). The whole themeing system is predicated on the idea that we can generate incomplete themes and then add them together via + or %+replace%.

We could let theme() take an argument providing the element tree or the theme to be modified, but that seems rather inelegant and cumbersome to the end user.

@hadley
Copy link
Member

@hadley hadley commented Jul 11, 2018

Hmmmm good point.

Could we move the inheritance into the elements? i.e. every element_() would gain an inherits argument? That would make creating a new theme considerably more complicated, but would make inheritance much simpler.

@nhamilton1980
Copy link
Author

@nhamilton1980 nhamilton1980 commented Jul 11, 2018

Gentlemen,

Thankyou very much for considering and working on this.

Could the element tree be stored at the plot level? Which would enable something like this:

ggplot() + 
   theme_add_element(new_axisA = .el_def("element_line", "axis.line", description="Axis 1"),
                     new_axisB = .el_def("element_line", "axis.line", description="Axis 2")) +
   theme_custom() +
   theme(new_axisA = element_line(size=2))

Each plot would contain and start with the default ggplot2 element tree.

It is up to the plot (or package) creator to define their new elements each time ggplot2 (or their dependent package) is used.

@clauswilke
Copy link
Member

@clauswilke clauswilke commented Jul 12, 2018

@nhamilton1980 I think your proposal would suffer from the same problem as making the element tree part of the theme: The blank theme function theme() wouldn't know about what has been defined at the plot level. I'll see if Hadley's suggestion about defining inheritance in the elements themselves is viable.

@nhamilton1980
Copy link
Author

@nhamilton1980 nhamilton1980 commented Jul 12, 2018

@clauswilke sorry, but wouldn't it be handled inside the add_ggplot(...) function found inside plot-construction.r? In which case the theme() function doesn't need to know what has or hasn't been added previously.

@clauswilke
Copy link
Member

@clauswilke clauswilke commented Jul 12, 2018

@nhamilton1980 No, themes can be added to each other, without having a ggplot object:

> theme(text = element_text(size = 10)) + theme(text = element_text(colour = "black"))
List of 1
 $ text:List of 11
  ..$ family       : NULL
  ..$ face         : NULL
  ..$ colour       : chr "black"
  ..$ size         : num 10
  ..$ hjust        : NULL
  ..$ vjust        : NULL
  ..$ angle        : NULL
  ..$ lineheight   : NULL
  ..$ margin       : NULL
  ..$ debug        : NULL
  ..$ inherit.blank: logi FALSE
  ..- attr(*, "class")= chr [1:2] "element_text" "element"
 - attr(*, "class")= chr [1:2] "theme" "gg"
 - attr(*, "complete")= logi FALSE
 - attr(*, "validate")= logi TRUE

@nhamilton1980
Copy link
Author

@nhamilton1980 nhamilton1980 commented Jul 12, 2018

@clauswilke I see. Good point.

@clauswilke
Copy link
Member

@clauswilke clauswilke commented Jul 12, 2018

@hadley I think adding inheritance info to each element will create a huge amount of trouble for existing 3rd-party theme code. I have an alternative idea, going back to keeping the inheritance info in the theme itself: What about verifying the theme only right before plot construction, when all parts of the theme are available? This means I'd be able to construct an inconsistent theme using just theme() functions, but once I tried to use that inconsistent theme in a plot I'd get an error.

@clauswilke

This comment has been minimized.

@clauswilke

This comment has been minimized.

@clauswilke

This comment has been minimized.

@hadley
Copy link
Member

@hadley hadley commented Jan 3, 2020

@clauswilke this made me realise that we should probably recommend that extension authors prefix any new theme settings with the name of their package in order to avoid clashes across packages.

@clauswilke

This comment has been minimized.

@hadley

This comment has been minimized.

@clauswilke
Copy link
Member

@clauswilke clauswilke commented Jan 3, 2020

Final demonstration of the new API, with recommended naming scheme for package developers.

library(ggplot2)

# Let's assume a package `ggxyz` wants to provide an easy way to add annotations to
# plot panels. To do so, it registers a new theme element `ggxyz.panel.annotation`
register_theme_elements(
  ggxyz.panel.annotation = element_text(color = "blue", hjust = 0.95, vjust = 0.05),
  element_tree = list(ggxyz.panel.annotation = el_def("element_text", "text"))
)

# Now the package can define a new coord that includes a panel annotation
coord_annotate <- function(label = "panel annotation") {
  ggproto(NULL, CoordCartesian,
          limits = list(x = NULL, y = NULL),
          expand = TRUE,
          default = FALSE,
          clip = "on",
          render_fg = function(panel_params, theme) {
            element_render(theme, "ggxyz.panel.annotation", label = label)
          }
  )
}

# Example plot with this new coord
df <- data.frame(x = 1:3, y = 1:3)
ggplot(df, aes(x, y)) +
  geom_point() +
  coord_annotate("annotation in blue")

# Revert to the original ggplot2 settings
reset_theme_settings()

Created on 2020-01-03 by the reprex package (v0.3.0)

@nhamilton1980
Copy link
Author

@nhamilton1980 nhamilton1980 commented Jan 3, 2020

@hadley, @clauswilke Thankyou both so much for this, I will integrate into the next ggtern release.

@clauswilke
Copy link
Member

@clauswilke clauswilke commented Jan 3, 2020

@nhamilton1980 As you prepare the next release of ggtern, please keep an eye out for other core components of ggplot2 that you have to duplicate or overwrite to make things work. Maybe we can address those in future releases of ggplot2. The goal should be that ggtern works entirely with the exported ggplot2 API, otherwise there'll always be headaches.

(A recent example: This bug is caused by a change to the internal Layer object in ggplot2:
https://bitbucket.org/nicholasehamilton/ggtern/issues/9/stat_sf-requires-the-following-missing )

@nhamilton1980
Copy link
Author

@nhamilton1980 nhamilton1980 commented Jan 4, 2020

I can think of one immediately.

In the plot construction routines, I think there should be hooks so that the layer can be rendered a) prior and b) after any coordinate transformations.

Many people have asked me to make ggrepel available, but it is difficult because that particular layer works almost exclusively in the cartesian space. I could make it work if I could somehow force it to operate on the data AFTER the simplex transformations have been performed.

@nhamilton1980
Copy link
Author

@nhamilton1980 nhamilton1980 commented Jan 4, 2020

I have mentioned before (I think leading up to my publication in JSS), that the required aesthetics / coordinates are not exclusively a function of the layer, but a layer in combination with the coordinate system. For example, A point in 1D requires x, in 2D requires x,y, in 3D requres x,y,z, and no I am not suggesting that ggplot2 should introduce 3d plotting, I am simply demonstrating that the required coordinates are a function of the layer operating in the specific coordinate system.

@clauswilke
Copy link
Member

@clauswilke clauswilke commented Jan 4, 2020

Please open separate issues for each of these items.

The issue of the relationship between position aesthetics and coords also recently came up in the context of geospatial data, see here: #3659 (comment). It's definitely something we need to look into further.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature internals 🔎 themes 💃
Projects
None yet
Development

No branches or pull requests

3 participants