Skip to content

allow empty facet specs #3162

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

Merged
merged 26 commits into from
Apr 11, 2019
Merged
Show file tree
Hide file tree
Changes from 18 commits
Commits
Show all changes
26 commits
Select commit Hold shift + click to select a range
f2aeb9d
Allow empty facetting
yutannihilation Feb 23, 2019
36829a8
Compact facet specs
yutannihilation Feb 23, 2019
81dcbc1
Use "(all)" for the dummy strip label
yutannihilation Feb 24, 2019
0e2d33f
Add wrap_as_facets_list()
yutannihilation Mar 2, 2019
8719f91
Compact in as_facets_list() and simplify grid_as_facets_list()
yutannihilation Mar 2, 2019
075c328
Reform some part of tests
yutannihilation Mar 2, 2019
e14a371
Fix check about aes()
yutannihilation Mar 2, 2019
45db931
Seperate validation
yutannihilation Mar 2, 2019
fb3ce9a
Do not compact in as_facets_list()
yutannihilation Mar 2, 2019
0d0dd33
Use more concrete facet specs
yutannihilation Mar 2, 2019
ba757c6
Fix tests
yutannihilation Mar 2, 2019
16e294d
Fix test about character
yutannihilation Mar 2, 2019
eda75d9
Compact NULL quosures
yutannihilation Mar 3, 2019
d17819f
Add tests about compacting specs
yutannihilation Mar 3, 2019
2d735e2
Include validation in as_facets_list()
yutannihilation Mar 3, 2019
675d1ac
Fix a typo
yutannihilation Mar 3, 2019
6537482
Remove a runtime assersion about grid facet specs
yutannihilation Mar 3, 2019
095a72d
Simplify the logic
yutannihilation Mar 3, 2019
fa856d0
Use expect_identical() for quosure tests
yutannihilation Mar 7, 2019
a7c017b
Improve comments
yutannihilation Mar 7, 2019
c0e4c76
Improve test cases
yutannihilation Mar 7, 2019
d90a160
Merge remote-tracking branch 'upstream/master' into feature/allow-emp…
yutannihilation Mar 7, 2019
7db3894
Add a NEWS bullet
yutannihilation Mar 7, 2019
2255be5
Merge remote-tracking branch 'upstream/master' into feature/allow-emp…
yutannihilation Mar 19, 2019
d6f58a6
Use new_quosures() instead of as_quosures()
yutannihilation Mar 19, 2019
1341f0b
Merge branch 'master' into feature/allow-empty-facets
thomasp85 Apr 11, 2019
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
27 changes: 11 additions & 16 deletions R/facet-.r
Original file line number Diff line number Diff line change
Expand Up @@ -275,10 +275,10 @@ df.grid <- function(a, b) {
# facetting variables.

as_facets_list <- function(x) {
if (inherits(x, "mapping")) {
stop("Please use `vars()` to supply facet variables")
if (inherits(x, "uneval")) {
stop("Please use `vars()` to supply facet variables", call. = FALSE)
}
if (inherits(x, "quosures")) {
if (rlang::is_quosures(x)) {
x <- rlang::quos_auto_name(x)
return(list(x))
}
Expand Down Expand Up @@ -311,13 +311,16 @@ as_facets_list <- function(x) {
x <- lapply(x, as_facets)
}

if (sum(vapply(x, length, integer(1))) == 0L) {
stop("Must specify at least one variable to facet by", call. = FALSE)
}

x
}

# flatten a list of quosures to a quosures object, and remove NULL quosure
compact_facets <- function(x) {
x <- rlang::flatten_if(x, rlang::is_list)
null <- vapply(x, rlang::quo_is_null, logical(1))
rlang::as_quosures(x[!null])
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this should be new_quosures(). The casting form converts formulas to quosures etc, and at this point we should have standardised the inputs already.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Makes sense. Thanks!

}

# Compatibility with plyr::as.quoted()
as_quoted <- function(x) {
if (is.character(x)) {
Expand Down Expand Up @@ -360,15 +363,7 @@ f_as_facets_list <- function(f) {
rows <- f_as_facets(lhs(f))
cols <- f_as_facets(rhs(f))

if (length(rows) + length(cols) == 0) {
stop("Must specify at least one variable to facet by", call. = FALSE)
}

if (length(rows)) {
list(rows, cols)
} else {
list(cols)
}
list(rows, cols)
}

as_facets <- function(x) {
Expand Down
51 changes: 26 additions & 25 deletions R/facet-grid-.r
Original file line number Diff line number Diff line change
Expand Up @@ -145,54 +145,46 @@ facet_grid <- function(rows = NULL, cols = NULL, scales = "fixed",
}

facets_list <- grid_as_facets_list(rows, cols)
n <- length(facets_list)
if (n > 2L) {
stop("A grid facet specification can't have more than two dimensions", call. = FALSE)
}
if (n == 1L) {
rows <- quos()
cols <- facets_list[[1]]
} else {
rows <- facets_list[[1]]
cols <- facets_list[[2]]
}

# Check for deprecated labellers
labeller <- check_labeller(labeller)

ggproto(NULL, FacetGrid,
shrink = shrink,
params = list(rows = rows, cols = cols, margins = margins,
params = list(rows = facets_list$rows, cols = facets_list$cols, margins = margins,
free = free, space_free = space_free, labeller = labeller,
as.table = as.table, switch = switch, drop = drop)
)
}

# returns a list containing exactly two quosures `rows` and `cols`
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Here it's not clear whether you're mentioning two single quosure or two quosure lists. I would use "quosure lists" or "lists of quosures" instead of just the plural "quosures".

Also we generally capitalise comments. No full stop, unless there are more than one sentence.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks! Actually I'm struggling about the wording here...

Also we generally capitalise comments. No full stop, unless there are more than one sentence.

I see.

grid_as_facets_list <- function(rows, cols) {
is_rows_vars <- is.null(rows) || rlang::is_quosures(rows)
if (!is_rows_vars) {
if (!is.null(cols)) {
stop("`rows` must be `NULL` or a `vars()` list if `cols` is a `vars()` list", call. = FALSE)
}
return(as_facets_list(rows))
# For backward-compatibility
facets_list <- as_facets_list(rows)
if (length(facets_list) > 2L) {
stop("A grid facet specification can't have more than two dimensions", call. = FALSE)
}
# fill with empty quosures
facets <- list(rows = rlang::quos(), cols = rlang::quos())
facets[seq_along(facets_list)] <- facets_list
# Do not compact the legacy specs
return(facets)
}

is_cols_vars <- is.null(cols) || rlang::is_quosures(cols)
if (!is_cols_vars) {
stop("`cols` must be `NULL` or a `vars()` specification", call. = FALSE)
}

if (is.null(rows)) {
rows <- quos()
} else {
rows <- rlang::quos_auto_name(rows)
}
if (is.null(cols)) {
cols <- quos()
} else {
cols <- rlang::quos_auto_name(cols)
}

list(rows, cols)
list(
rows = compact_facets(as_facets_list(rows)),
cols = compact_facets(as_facets_list(cols))
)
}

#' @rdname ggplot2-ggproto
Expand Down Expand Up @@ -223,6 +215,10 @@ FacetGrid <- ggproto("FacetGrid", Facet,
base_cols <- combine_vars(data, params$plot_env, cols, drop = params$drop)
base <- df.grid(base_rows, base_cols)

if (nrow(base) == 0) {
return(new_data_frame(list(PANEL = 1L, ROW = 1L, COL = 1L, SCALE_X = 1L, SCALE_Y = 1L)))
}

# Add margins
base <- reshape2::add_margins(base, list(names(rows), names(cols)), params$margins)
# Work around bug in reshape2
Expand Down Expand Up @@ -253,6 +249,11 @@ FacetGrid <- ggproto("FacetGrid", Facet,
cols <- params$cols
vars <- c(names(rows), names(cols))

if (length(vars) == 0) {
data$PANEL <- layout$PANEL
return(data)
}

# Compute faceting values and add margins
margin_vars <- list(intersect(names(rows), names(data)),
intersect(names(cols), names(data)))
Expand Down
22 changes: 19 additions & 3 deletions R/facet-wrap.r
Original file line number Diff line number Diff line change
Expand Up @@ -109,8 +109,7 @@ facet_wrap <- function(facets, nrow = NULL, ncol = NULL, scales = "fixed",
labeller <- check_labeller(labeller)

# Flatten all facets dimensions into a single one
facets_list <- as_facets_list(facets)
facets <- rlang::flatten_if(facets_list, rlang::is_list)
facets <- wrap_as_facets_list(facets)

ggproto(NULL, FacetWrap,
shrink = shrink,
Expand All @@ -128,6 +127,12 @@ facet_wrap <- function(facets, nrow = NULL, ncol = NULL, scales = "fixed",
)
}

# returns quosures
wrap_as_facets_list <- function(x) {
facets_list <- as_facets_list(x)
compact_facets(facets_list)
}

#' @rdname ggplot2-ggproto
#' @format NULL
#' @usage NULL
Expand Down Expand Up @@ -177,8 +182,14 @@ FacetWrap <- ggproto("FacetWrap", Facet,
if (empty(data)) {
return(cbind(data, PANEL = integer(0)))
}

vars <- params$facets

if (length(vars) == 0) {
data$PANEL <- 1L
return(data)
}

facet_vals <- eval_facets(vars, data, params$plot_env)
facet_vals[] <- lapply(facet_vals[], as.factor)

Expand Down Expand Up @@ -229,7 +240,12 @@ FacetWrap <- ggproto("FacetWrap", Facet,

axes <- render_axes(ranges, ranges, coord, theme, transpose = TRUE)

labels_df <- layout[names(params$facets)]
if (length(params$facets) == 0) {
# add a dummy label
labels_df <- new_data_frame(list("(all)" = "(all)"), n = 1)
} else {
labels_df <- layout[names(params$facets)]
}
attr(labels_df, "facet") <- "wrap"
strips <- render_strips(
structure(labels_df, type = "rows"),
Expand Down
45 changes: 35 additions & 10 deletions tests/testthat/test-facet-.r
Original file line number Diff line number Diff line change
@@ -1,9 +1,8 @@
context("Facetting")

test_that("as_facets_list() coerces formulas", {
expect_identical(as_facets_list(~foo), list(quos(foo = foo)))
expect_identical(as_facets_list(~foo + bar), list(quos(foo = foo, bar = bar)))

expect_identical(as_facets_list(~foo), list(quos(), quos(foo = foo)))
expect_identical(as_facets_list(~foo + bar), list(quos(), quos(foo = foo, bar = bar)))
expect_identical(as_facets_list(foo ~ bar), list(quos(foo = foo), quos(bar = bar)))

exp <- list(quos(foo = foo, bar = bar), quos(baz = baz, bam = bam))
Expand All @@ -13,13 +12,24 @@ test_that("as_facets_list() coerces formulas", {
expect_identical(as_facets_list(foo() + bar() ~ baz() + bam()), exp)
})

test_that("wrap_as_facets_list() returns quosures", {
expect_identical(wrap_as_facets_list(~foo), quos(foo = foo))
expect_identical(wrap_as_facets_list(~foo + bar), quos(foo = foo, bar = bar))
expect_identical(wrap_as_facets_list(foo ~ bar), quos(foo = foo, bar = bar))
})

test_that("as_facets_list() coerces strings containing formulas", {
expect_identical(as_facets_list("foo ~ bar"), as_facets_list(local(foo ~ bar, globalenv())))
})

test_that("as_facets_list() coerces character vectors", {
expect_identical(as_facets_list("foo"), as_facets_list(local(~foo, globalenv())))
expect_identical(as_facets_list(c("foo", "bar")), as_facets_list(local(foo ~ bar, globalenv())))
foo <- rlang::new_quosure(quote(foo), globalenv())
bar <- rlang::new_quosure(quote(bar), globalenv())
foobar <- rlang::as_quosures(list(foo, bar), named = TRUE)

expect_identical(as_facets_list("foo"), list(foobar[1]))
expect_identical(as_facets_list(c("foo", "bar")), list(foobar[1], foobar[2]))
expect_identical(wrap_as_facets_list(c("foo", "bar")), foobar)
})

test_that("as_facets_list() coerces lists", {
Expand All @@ -36,17 +46,32 @@ test_that("as_facets_list() coerces lists", {
expect_identical(out, exp)
})

test_that("as_facets_list() errors with empty specs", {
expect_error(as_facets_list(list()), "at least one variable to facet by")
expect_error(as_facets_list(. ~ .), "at least one variable to facet by")
expect_error(as_facets_list(list(. ~ .)), "at least one variable to facet by")
expect_error(as_facets_list(list(NULL)), "at least one variable to facet by")
test_that("wrap_as_facets_list() and grid_as_facets_list() accept empty specs", {
expect_equal(wrap_as_facets_list(NULL), quos())
expect_equal(wrap_as_facets_list(list()), quos())
expect_equal(wrap_as_facets_list(. ~ .), quos())
expect_equal(wrap_as_facets_list(list(. ~ .)), quos())
expect_equal(wrap_as_facets_list(list(NULL)), quos())

expect_equal(grid_as_facets_list(list(), NULL), list(rows = quos(), cols = quos()))
expect_equal(grid_as_facets_list(. ~ ., NULL), list(rows = quos(), cols = quos()))
expect_equal(grid_as_facets_list(list(. ~ .), NULL), list(rows = quos(), cols = quos()))
expect_equal(grid_as_facets_list(list(NULL), NULL), list(rows = quos(), cols = quos()))
})

test_that("wrap_as_facets_list() and grid_as_facets_list() compact specs", {
expect_equal(wrap_as_facets_list(vars(foo, NULL, bar)), quos(foo = foo, bar = bar))
expect_equal(grid_as_facets_list(vars(foo, NULL, bar), NULL), list(rows = quos(foo = foo, bar = bar), cols = quos()))
})

test_that("as_facets_list() coerces quosure lists", {
expect_identical(as_facets_list(vars(foo)), list(rlang::quos(foo = foo)))
})

test_that("facets rejects aes()", {
expect_error(facet_wrap(aes(foo)), "Please use `vars()` to supply facet variables", fixed = TRUE)
expect_error(facet_grid(aes(foo)), "Please use `vars()` to supply facet variables", fixed = TRUE)
})

df <- data_frame(x = 1:3, y = 3:1, z = letters[1:3])

Expand Down