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

Add plot layout summary API #2034

Merged
merged 20 commits into from Feb 9, 2017
Merged

Add plot layout summary API #2034

merged 20 commits into from Feb 9, 2017

Conversation

@wch
Copy link
Member

@wch wch commented Feb 7, 2017

Examples:

p <- ggplot(mpg, aes(displ, hwy)) + geom_point()
ggplot_build(p)$layout$get_layout_summary()
# # A tibble: 1 × 10
# panel   row   col       vars  xmin  xmax  ymin  ymax                        xscale
# <fctr> <dbl> <dbl>     <list> <dbl> <dbl> <dbl> <dbl>                        <list>
#   1      1     1     1 <list [0]>  1.33  7.27  10.4  45.6 <S3: ScaleContinuousPosition>
#   # ... with 1 more variables: yscale <list>


pw <- p + facet_wrap(~ drv)
ggplot_build(pw)$layout$get_layout_summary()
# # A tibble: 3 × 10
# panel   row   col       vars  xmin  xmax  ymin  ymax                        xscale
# <fctr> <int> <int>     <list> <dbl> <dbl> <dbl> <dbl>                        <list>
#   1      1     1     1 <list [1]>  1.33  7.27  10.4  45.6 <S3: ScaleContinuousPosition>
#   2      2     1     2 <list [1]>  1.33  7.27  10.4  45.6 <S3: ScaleContinuousPosition>
#   3      3     1     3 <list [1]>  1.33  7.27  10.4  45.6 <S3: ScaleContinuousPosition>
#   # ... with 1 more variables: yscale <list>
  

pg <- p + facet_grid(drv ~ cyl)
ggplot_build(pg)$layout$get_layout_summary()
# # A tibble: 12 × 10
# panel   row   col       vars  xmin  xmax  ymin  ymax                        xscale
# <fctr> <int> <int>     <list> <dbl> <dbl> <dbl> <dbl>                        <list>
#   1       1     1     1 <list [2]>  1.33  7.27  10.4  45.6 <S3: ScaleContinuousPosition>
#   2       2     1     2 <list [2]>  1.33  7.27  10.4  45.6 <S3: ScaleContinuousPosition>
#   3       3     1     3 <list [2]>  1.33  7.27  10.4  45.6 <S3: ScaleContinuousPosition>
#   4       4     1     4 <list [2]>  1.33  7.27  10.4  45.6 <S3: ScaleContinuousPosition>
#   5       5     2     1 <list [2]>  1.33  7.27  10.4  45.6 <S3: ScaleContinuousPosition>
#   6       6     2     2 <list [2]>  1.33  7.27  10.4  45.6 <S3: ScaleContinuousPosition>
#   7       7     2     3 <list [2]>  1.33  7.27  10.4  45.6 <S3: ScaleContinuousPosition>
#   8       8     2     4 <list [2]>  1.33  7.27  10.4  45.6 <S3: ScaleContinuousPosition>
#   9       9     3     1 <list [2]>  1.33  7.27  10.4  45.6 <S3: ScaleContinuousPosition>
#   10     10     3     2 <list [2]>  1.33  7.27  10.4  45.6 <S3: ScaleContinuousPosition>
#   11     11     3     3 <list [2]>  1.33  7.27  10.4  45.6 <S3: ScaleContinuousPosition>
#   12     12     3     4 <list [2]>  1.33  7.27  10.4  45.6 <S3: ScaleContinuousPosition>
#   # ... with 1 more variables: yscale <list>
R/facet-null.r Outdated
@@ -30,8 +30,10 @@ FacetNull <- ggproto("FacetNull", Facet,
# Need the is.waive check for special case where no data, but aesthetics
# are mapped to vectors
if (is.waive(data) || empty(data))
return(cbind(data, PANEL = integer(0)))
data$PANEL <- 1L
return(cbind(data, PANEL = factor()))
Copy link
Member

@hadley hadley Feb 7, 2017

Looking at it afresh, this code doesn't seem right. Why are we cbinding to something that's absent?

Copy link
Member Author

@wch wch Feb 7, 2017

I'm not 100% sure.. Maybe it makes sense for zero-row data?

Copy link
Member

@hadley hadley Feb 7, 2017

Maybe is.waive() should get its own branch?

Copy link
Member Author

@wch wch Feb 8, 2017

If I change it to this:

    if (is.waive(data))
      return(data)
    if (empty(data))
      return(cbind(data, PANEL = factor()))

It errors on this plot:

ggplot() + geom_point(aes(1:5, 1:5))
# Error in if (n == 0) { : argument is of length zero 

It appears that when it's a waiver object, still requires the PANEL to be cbinded to it.

> cbind(waiver(), PANEL=factor())
      PANEL
> str(cbind(waiver(), PANEL=factor()))
 list()
 - attr(*, "dim")= int [1:2] 0 2
 - attr(*, "dimnames")=List of 2
  ..$ : NULL
  ..$ : chr [1:2] "" "PANEL"

Should I just leave as is?

Copy link
Member

@hadley hadley Feb 8, 2017

Sorry, missed this. I think you should just return tibble(PANEL = factor()) for the waiver case

R/layout.R Outdated

self$layer_mappings <- lapply(plot$layers, function(layer) {
mapping <- default
default[names(layer$mapping)] <- layer$mapping
Copy link
Member

@hadley hadley Feb 7, 2017

Can you use modifyList() here?

Copy link
Member

@hadley hadley Feb 7, 2017

Also check that there isn't an existing function that does this (by grepping for modifyList())

Copy link
Member Author

@wch wch Feb 7, 2017

I can use modifyList, but since layer$mapping can be NULL, it also needs a is.null check.

Grepping for modifyList doesn't turn up anything that already does this.

Copy link
Member

@hadley hadley Feb 7, 2017

Oh I used to use plyr::defaults()

R/layout.R Outdated
},

get_layout_summary = function(self) {
layout <- self$layout
Copy link
Member

@hadley hadley Feb 7, 2017

Rather than aggressively modifying this data frame, might be simpler to start from scratch with a new one?

Copy link
Member Author

@wch wch Feb 7, 2017

Sure.

R/layout.R Outdated
# Given a transform object, find the log base; if the transform object is
# NULL, or if it's not a log transform, return NA.
trans_get_log_base <- function(trans) {
if (!is.null(trans) && grepl("^log-", trans$name))
Copy link
Member

@hadley hadley Feb 7, 2017

Can you add {} please? Or do an early return.

R/layout.R Outdated
# NULL, or if it's not a log transform, return NA.
trans_get_log_base <- function(trans) {
if (!is.null(trans) && grepl("^log-", trans$name))
as.numeric(sub("^log-", "", trans$name))
Copy link
Member

@hadley hadley Feb 7, 2017

It would be slightly cleaner to do environment(trans$transform)$base

list(data = data, layout = layout, plot = plot)
structure(
list(data = data, layout = layout, plot = plot),
class = "ggplot_built"
Copy link
Member

@hadley hadley Feb 7, 2017

In ggplot_gtable(), do you mind adding a check for this class?

Copy link
Member Author

@wch wch Feb 8, 2017

Done.

@@ -175,7 +175,7 @@ print.ggplot <- function(x, newpage = is.null(vp), vp = NULL, ...) {
upViewport()
}

invisible(data)
invisible(x)
Copy link
Member

@hadley hadley Feb 7, 2017

This needs a NEWS bullet

lw <- ggplot_build(pw)$layout$get_layout_summary()
lg <- ggplot_build(pg)$layout$get_layout_summary()

# Basic tests
Copy link
Member

@hadley hadley Feb 7, 2017

Should be one test for each block

hadley
hadley approved these changes Feb 8, 2017
Copy link
Member

@hadley hadley left a comment

Feel free to merge once you've fixed that last tiny thing.

R/layout.R Outdated
mapping <- default
default[names(layer$mapping)] <- layer$mapping
mapping
defaults(layer$mapping, default)
Copy link
Member

@hadley hadley Feb 8, 2017

I think you can now eliminate the anonymous function too?

Copy link
Member Author

@wch wch Feb 8, 2017

I don't think so, because the defaults function gets passed layer$mapping, not layer.

Copy link
Member

@hadley hadley Feb 8, 2017

Oh yeah

@wch wch force-pushed the plot-info-api branch from ab2abbd to a3a68cc Feb 8, 2017
@wch wch merged commit f97f71e into tidyverse:master Feb 9, 2017
2 checks passed
@wch wch deleted the plot-info-api branch Feb 9, 2017
@lock
Copy link

@lock lock bot commented Jan 18, 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 Jan 18, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

2 participants