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

Allow creation of custom layers that have access to global plot data #2875

Merged
merged 6 commits into from
Nov 15, 2018

Conversation

clauswilke
Copy link
Member

@clauswilke clauswilke commented Aug 30, 2018

This PR creates a general framework for custom layers that have access to the global plot data. It provides a solution to the problem that we're combining layers with + instead of %>%, and it closes #2872.

R/sf.R Outdated Show resolved Hide resolved
@yutannihilation
Copy link
Member

In general, looks good to me!

@hadley
Copy link
Member

hadley commented Aug 31, 2018

Doesn't this assume that the plot data is already set at the time the layer is added? That's been the stumbling block for my previous attempts to fix the problem (and is a design decision that I now regret)

@clauswilke
Copy link
Member Author

@hadley I'm not sure I fully understand the question. As far as I understand, the data for a layer can come either from the original ggplot() call or from the data argument in the layer. My function setup_layer() has access to both and uses the layer_data() function to figure out which is the right one. The layer_data() is used later again to figure out the data for each layer, so the behavior should be identical. (And there is room for optimization so it's called only once, but layer_data() is pretty lightweight so I don't see a big need for this optimization at this time.)

@hadley
Copy link
Member

hadley commented Aug 31, 2018

What about the case where the data is added later?

ggplot() + geom_sf() + aes(x, y) %+% data

@clauswilke
Copy link
Member Author

I wasn't aware of the %+% data option. Let me ponder it. I think you're right that that scenario can't be handled with my current proposal, but it may be possible to move the setup_layer() call to later in the toolchain where this would work also.

@clauswilke
Copy link
Member Author

The setup_layer() function was called in the wrong location. It needs to be called from within ggplot_build(). Then it's possible to add data at the very end and still have all layers behave correctly.

library(tidyverse)

## SF example
# Geometry column is called "geom". It is found correctly.
nc_gpkg <- sf::st_read(system.file("gpkg/nc.gpkg", package = "sf"), quiet = TRUE)
ggplot(nc_gpkg) + geom_sf() + aes(fill = AREA)

# even if we add the data at the very end
(ggplot() + geom_sf() + aes(fill = AREA)) %+% nc_gpkg 

## Grouping example
# grouping ignored
group_by(iris, Species) %>%
  ggplot(aes(Sepal.Length, Sepal.Width)) + geom_smooth(method = "lm")

# autogrouping function
setup_autogrp = function(self, data, plot) {
  if ((isTRUE(self$inherit.aes) && is.null(self$mapping$group) && is.null(plot$mapping$group)) ||
      (!isTRUE(self$inherit.aes) && is.null(self$mapping$group))) {
    if (!is.null(data$.group)) {
      self$mapping$group <- as.name(".group")
    }
  }
  data
}

# now grouping is used but only in that layer
smooth_grp_layer <- geom_smooth(method = "lm")
smooth_grp_layer$setup_layer <- setup_autogrp
group_by(iris, Species) %>%
  ggplot(aes(Sepal.Length, Sepal.Width)) + 
    smooth_grp_layer +
    geom_text(
      data = tibble(Sepal.Length = 5.5, Sepal.Width = 3.25, label = "A label."),
      aes(label = label)
    )

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

@clauswilke
Copy link
Member Author

clauswilke commented Sep 1, 2018

One more example, doing the kind of filtering operation needed for gghighlight.

library(ggplot2)
# filtering function
setup_filter = function(self, data, plot) {
  # example filter
  filter <- quo(mpg > 25)

  # filter data
  index <- rlang::eval_tidy(filter, data)
  data[index, ]
}

# points that meet the filter criterion are shown in red
point_filter_layer <- geom_point(color = "red")
point_filter_layer$setup_layer <- setup_filter
ggplot(mtcars, aes(hp, mpg)) + 
  geom_point() +
  point_filter_layer

# works also with data added at the end
(ggplot() +
  aes(hp, mpg) +
  geom_point() +
  point_filter_layer) %+% mtcars

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

@yutannihilation
Copy link
Member

Thanks for the gghighlight example! Seems very useful. 👍

@clauswilke
Copy link
Member Author

@hadley Now that 3.1.0 is out of the way, I was wondering whether we can go ahead with the PR. I think the latest version gets the implementation right and addresses your concern from Aug. 31 about data being added later. Overall, this is a very small code modification that will open up a wide range of new extension capabilities (see examples of autogrouping layers and highlighting layers provided above).

Might be good to get @thomasp85's input as well. Are there any implications for gganimate?

@thomasp85
Copy link
Member

In general the changes looks benign, and I can't see how it would affect gganimate (other than I have to update ggplot_build.gganim() to reflect these additions...

I'm pretty sure someone will use it for something that will break something at some point, but that's for future us to worry about 😄

LGTM

@clauswilke
Copy link
Member Author

@thomasp85 We should coordinate carefully to avoid breaking gganimate with the currently released ggplot2 and/or with github master. I think this means patching gganimate first. I can prepare a PR.

I see two ways to do this. Any preference?

# version 1: explicit version check
if (packageVersion("ggplot2") <= "3.1.0") {
  # old code
} else {
  # new code
}

# version 2: check for capabilities
data <- layer_data
data <- by_layer(
  function(l, d) {
    # setup_layer() function doesn't exist in ggplot2 3.1.0 or earlier
    if (is.null(l$setup_layer)) d else l$setup_layer(d, plot)
  }
)

Does the same issue arise with patchwork?

@thomasp85
Copy link
Member

I think version 1 is preferable as it self-document why this complication is added (and can be removed in time with little worry)

In fact if the next version of ggplot2 hits cran before gganimate is released I might not include backward compatibility (but I'll worry about that then)

Patchwork does not reimplement the ggplot_build() method but simply calls NextMethod(), so it should be insulated from changes in ggplot2

@thomasp85
Copy link
Member

Down the line it would be nice if we could figure out a way that didn't require gganimate to copy large swaths of ggplot2 code in order to insert itself the right places, but that is for another day to worry about

@clauswilke
Copy link
Member Author

@thomasp85 I realized that gganimate will mostly continue to work without the fix, so I'm merging the change here now. I also prepared a PR for gganimate.

@clauswilke clauswilke merged commit 4380cb9 into tidyverse:master Nov 15, 2018
@clauswilke clauswilke deleted the issue-2872-geometry-mapping branch November 15, 2018 21:35
@yutannihilation yutannihilation mentioned this pull request Mar 28, 2019
cpsievert added a commit to plotly/plotly.R that referenced this pull request Apr 7, 2019
@lock
Copy link

lock bot commented May 14, 2019

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 May 14, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Reduce "object 'geometry' not found" error with geom_sf()
4 participants