-
Notifications
You must be signed in to change notification settings - Fork 2.1k
Enable User Defined Theme Elements #2540
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
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 all you need is the ability to add new theme elements, that functionality can be achieved with one new function,
Now, we can do:
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? |
Is this the same as #1730? |
This comment has been minimized.
This comment has been minimized.
Claus,
Thanks, sure perhaps my proposed solution was a bit over the top, I am really not too fussed how the end goal is achieved. Your solution seems simpler, I agree, however 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.
|
Nicholas,
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? |
Yes that would be fine.
|
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. |
Fantastic. This will be extremely beneficial to me. |
See also #2649 for a different problem that requires a similar technical fix: a currently hard-coded list needs to become user-configurable. |
@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: Lines 3 to 4 in 3d022ed
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.
|
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. |
@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. |
@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 Specifically, consider the following example. Let's say I have defined a theme theme_custom() + theme(some.box = element_box(color = "red)) I don't see how the We could let |
Hmmmm good point. Could we move the inheritance into the elements? i.e. every |
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:
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. |
@nhamilton1980 I think your proposal would suffer from the same problem as making the element tree part of the theme: The blank theme function |
@clauswilke sorry, but wouldn't it be handled inside the |
@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 |
@clauswilke I see. Good point. |
@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 |
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
@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. |
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
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) |
@hadley, @clauswilke Thankyou both so much for this, I will integrate into the next ggtern release. |
@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 |
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. |
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. |
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. |
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.
The text was updated successfully, but these errors were encountered: