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

Rework simplification #909

Merged
merged 38 commits into from
Sep 12, 2022
Merged
Show file tree
Hide file tree
Changes from 6 commits
Commits
Show all changes
38 commits
Select commit Hold shift + click to select a range
bcc5415
Implement list_simplify() and use it in accumulate()
hadley Aug 31, 2022
1ead93b
Try a strict argument
hadley Aug 31, 2022
a927496
Tweak simplify spec
hadley Sep 2, 2022
be3fe11
Finish off list_transpose()
hadley Sep 2, 2022
17bffab
Use vec_unchop()
hadley Sep 2, 2022
8d63020
Remove out of date comment
hadley Sep 2, 2022
02ea731
Merge commit 'a118aeca0768493da48058882598389521fa3558'
hadley Sep 7, 2022
71ba50c
Feedback from code review
hadley Sep 7, 2022
20387d0
Finish off list_simplify() tests
hadley Sep 7, 2022
7805591
Update accumulate tests
hadley Sep 7, 2022
5d6d874
Basic docs for list_transpose()
hadley Sep 7, 2022
d9c76d3
Mildly consider simplification errors
hadley Sep 7, 2022
1980e24
Test list_transpose()
hadley Sep 7, 2022
eb85559
Deprecate transpose()
hadley Sep 7, 2022
5be021d
Merge commit '1cf95cbdf0fdcff27cad77b9a5f791d00107ced4'
hadley Sep 9, 2022
5940cc2
Replace accidental use of base pipe
hadley Sep 9, 2022
675a8a8
Implement user facing list_simplify()
hadley Sep 9, 2022
8b32bfa
Deprecate as_vector(), simplify(), simplify_all()
hadley Sep 9, 2022
a4f864d
Add news bullets
hadley Sep 9, 2022
822d714
Use cli for tests
hadley Sep 9, 2022
5254896
Apply suggestions from code review
hadley Sep 12, 2022
c8b4bc9
Merge commit '95a568c2bb8a5a7c0c7a70b02fc233d0a2c4ca02'
hadley Sep 12, 2022
e4311be
Re-document
hadley Sep 12, 2022
80fa90c
Error tweaking
hadley Sep 12, 2022
41b2039
Simplify simplify errors
hadley Sep 12, 2022
a7a41c8
More code review feedback
hadley Sep 12, 2022
4577473
Let list_transpose() work with numeric templates
hadley Sep 12, 2022
5319949
Add more transpose examples
hadley Sep 12, 2022
fcb92d6
Remove unnused error_arg
hadley Sep 12, 2022
2397f21
Avoid offense to the delicate senisbilities of Lionel and Davis
hadley Sep 12, 2022
471f340
Merge commit 'ff4dfcb16d64ed80bbf34e32c1ac4eff002ba11e'
hadley Sep 12, 2022
b16071a
Merge commit '61fb2accc032fab3ff3b2012bab8948194e8d08f'
hadley Sep 12, 2022
da1f7c9
Apply suggestions from code review
hadley Sep 12, 2022
0590ccf
Re-document & update snapshots
hadley Sep 12, 2022
99c925e
Improve list_simplify() errors + docs
hadley Sep 12, 2022
808568b
list_transpose() improvements
hadley Sep 12, 2022
2c5866a
Tweak docs
hadley Sep 12, 2022
78938f7
Move accidental change
hadley Sep 12, 2022
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
40 changes: 40 additions & 0 deletions R/list-simplify.R
Original file line number Diff line number Diff line change
@@ -0,0 +1,40 @@
list_simplify <- function(x, simplify = NA, ptype = NULL) {
hadley marked this conversation as resolved.
Show resolved Hide resolved
vec_assert(x, list())
hadley marked this conversation as resolved.
Show resolved Hide resolved
if (length(simplify) > 1 || !is.logical(simplify)) {
abort("`simplify` must be `TRUE`, `FALSE`, or `NA`")
}
if (!is.null(ptype) && isFALSE(simplify)) {
abort("Must not specify `ptype` when `simplify = FALSE`")
}

if (isFALSE(simplify)) {
hadley marked this conversation as resolved.
Show resolved Hide resolved
return(x)
}
strict <- !is.na(simplify)

# We choose not to simply data frames to preserve length invariants
hadley marked this conversation as resolved.
Show resolved Hide resolved
can_simplify <- every(x, ~ vec_is(.x, size = 1) && !is.data.frame(.x))

if (can_simplify) {
if (!is.null(ptype)) {
vec_unchop(x, ptype = ptype)
} else {
tryCatch(
vec_unchop(x),
vctrs_error_incompatible_type = function(err) {
if (strict) {
abort("Failed to simplify", parent = err)
} else {
x
}
}
)
}
} else {
if (strict) {
abort("Failed to simplify: not all elements vectors of length 1")
} else {
x
}
}
}
66 changes: 66 additions & 0 deletions R/list-transpose.R
Original file line number Diff line number Diff line change
@@ -0,0 +1,66 @@
#' @examples
#' # list_tranpose() is useful in conjunction with safely()
#' x <- list("a", 1, 2)
#' y <- x %>% map(safely(log))
#' y %>% str()
#' y %>% list_transpose() %>% str()
#' y %>% list_transpose(default = list(result = NA)) %>% str()
#'
#' # list_tranpose() will try to simplify by default:
#' x <- list(list(a = 1, b = 2), list(a = 3, b = 4), list(a = 5, b = 6))
#' x %>% list_transpose()
#' # use simplify = FALSE to always return lists:
#' x %>% list_transpose(simplify = FALSE) |> str()
#'
#' # Provide explicit component names to prevent loss of those that don't
#' # appear in first component
#' ll <- list(
#' list(x = 1, y = "one"),
#' list(z = "deux", x = 2)
#' )
#' ll %>% list_transpose()
#' ll %>% list_transpose(c("x", "y", "z"))
#'
#' # And specify default if you want to simplify
hadley marked this conversation as resolved.
Show resolved Hide resolved
#' ll %>% list_transpose(c("x", "y", "z"), default = NA)
list_transpose <- function(x, template = NULL, simplify = NA, ptype = NULL, default = NULL) {
vec_assert(x, list())
if (length(x) == 0) {
return(list())
}

template <- template %||%
names(x[[1]]) %||%
cli::cli_abort("First element of {.arg x} is unnamed, please supply `template.")
if (!is.character(template)) {
cli::cli_abort("{.arg template} must be a character vector")
}

simplify <- match_template(simplify, template)
default <- match_template(default, template)
ptype <- match_template(ptype, template)

out <- rep_named(template, list())
for (nm in template) {
res <- map(x, nm, .default = default[[nm]])
res <- list_simplify(res, simplify = simplify[[nm]], ptype = ptype[[nm]])
out[[nm]] <- res
}

out
}

match_template <- function(x, template, error_arg = caller_arg(x), error_call = caller_env()) {
if (is_bare_list(x) && is_named(x)) {
extra_names <- setdiff(names(x), template)
if (length(extra_names)) {
cli::cli_abort(
hadley marked this conversation as resolved.
Show resolved Hide resolved
"{.arg {error_arg}} contains unknown names: {.str extra_names}",
call = error_call
)
}
x
} else {
rep_named(template, list(x))
}
}
26 changes: 13 additions & 13 deletions R/reduce.R
Original file line number Diff line number Diff line change
Expand Up @@ -343,11 +343,15 @@ seq_len2 <- function(start, end) {
#' the accumulation, rather than using `.x[[1]]`. This is useful if
#' you want to ensure that `reduce` returns a correct value when `.x`
#' is empty. If missing, and `.x` is empty, will throw an error.
#'
#' @param .dir The direction of accumulation as a string, one of
#' `"forward"` (the default) or `"backward"`. See the section about
#' direction below.
#'
#' @param .simplify If `NA`, the default, the accumulated list of
#' results is simplified to an atomic vector if possible.
#' If `TRUE`, the result is simplified, erroring if not possible.
#' If `FALSE`, the result is not simplified, always returning a list.
#' @param .ptype If `simplify` is `TRUE`, optionally supply a vector prototype
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
#' @param .ptype If `simplify` is `TRUE`, optionally supply a vector prototype
#' @param .ptype If `.simplify` is `TRUE`, optionally supply a vector prototype

Copy link
Member

Choose a reason for hiding this comment

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

It also works if .simplify is NA right?

#' to enforce the output types.
#' @return A vector the same length of `.x` with the same names as `.x`.
#'
#' If `.init` is supplied, the length is extended by 1. If `.x` has
Expand Down Expand Up @@ -454,26 +458,22 @@ seq_len2 <- function(start, end) {
#' ggtitle("Simulations of a random walk with drift")
#' }
#' @export
accumulate <- function(.x, .f, ..., .init, .dir = c("forward", "backward")) {
accumulate <- function(.x, .f, ..., .init, .dir = c("forward", "backward"), .simplify = NA, .ptype = NULL) {
.dir <- arg_match(.dir, c("forward", "backward"))
.f <- as_mapper(.f, ...)

res <- reduce_impl(.x, .f, ..., .init = .init, .dir = .dir, .acc = TRUE)
names(res) <- accumulate_names(names(.x), .init, .dir)

# It would be unappropriate to simplify the result rowwise with
# `accumulate()` because it has invariants defined in terms of
# `length()` rather than `vec_size()`
if (some(res, is.data.frame)) {
res
} else {
vec_simplify(res)
}
res <- list_simplify(res, .simplify, .ptype)
res
}
#' @rdname accumulate
#' @export
accumulate2 <- function(.x, .y, .f, ..., .init) {
reduce2_impl(.x, .y, .f, ..., .init = .init, .acc = TRUE)
accumulate2 <- function(.x, .y, .f, ..., .init, .simplify = NA, .ptype = NULL) {
res <- reduce2_impl(.x, .y, .f, ..., .init = .init, .acc = TRUE)
res <- list_simplify(res, .simplify, .ptype)
res
}

accumulate_names <- function(nms, init, dir) {
Expand Down
15 changes: 0 additions & 15 deletions R/utils.R
Original file line number Diff line number Diff line change
Expand Up @@ -280,21 +280,6 @@ quo_invert <- function(call) {
quo_is_constant <- function(quo) {
is_reference(quo_get_env(quo), empty_env())
}

vec_simplify <- function(x) {
if (!vctrs::vec_is_list(x)) {
return(x)
}
if (!every(x, ~ vctrs::vec_is(.x) && vctrs::vec_size(.x) == 1L)) {
return(x)
}

tryCatch(
vctrs_error_incompatible_type = function(...) x,
vctrs::vec_c(!!!x)
)
}

quo_is_same_env <- function(x, env) {
quo_env <- quo_get_env(x)
is_reference(quo_env, env) || is_reference(quo_env, empty_env())
Expand Down
22 changes: 19 additions & 3 deletions man/accumulate.Rd

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

17 changes: 17 additions & 0 deletions tests/testthat/_snaps/list-simplify.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,17 @@
# ptype is checked

Code
list_simplify(list(1, 2), ptype = character())
Condition
Error:
! Can't convert <double> to <character>.
Comment on lines +6 to +7
Copy link
Member

Choose a reason for hiding this comment

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

@lionel- I think this error is being thrown from this pattern:

    tryCatch(
      # TODO: use `error_call` when available
      list_unchop(x, ptype = ptype),
      vctrs_error_incompatible_type = function(err) {
        if (strict || !is.null(ptype)) {
          cnd_signal(err)
        } else {
          x
        }
      }
    )

Do you know why it doesn't show the function call? i.e. list_unchop()?


# x must be a list

Code
list_simplify(1:5)
Condition
Error in `list_simplify()`:
! `x` must be a vector with type <list>.
Instead, it has type <integer>.

30 changes: 30 additions & 0 deletions tests/testthat/test-list-simplify.R
Original file line number Diff line number Diff line change
@@ -0,0 +1,30 @@
test_that("simplification requires vectors", {
expect_equal(list_simplify(list(mean)), list(mean))
})

test_that("simplification requires length 1 inputs", {
expect_equal(list_simplify(list(1, 2:3)), list(1, 2:3))
expect_equal(list_simplify(list(1, 2, 3)), c(1, 2, 3))
})

test_that("simplification requires common type", {
expect_equal(list_simplify(list(1, 2)), c(1, 2))
expect_equal(list_simplify(list(1, "a")), list(1, "a"))
})

test_that("never simplifies data frames", {
x <- list(data.frame(x = 1), data.frame(y = 1))
expect_equal(list_simplify(x), x)
})

test_that("ptype is checked", {
expect_equal(list_simplify(list(1, 2), ptype = double()), c(1, 2))
expect_snapshot(list_simplify(list(1, 2), ptype = character()), error = TRUE)
})


# argument checking -------------------------------------------------------

test_that("x must be a list", {
expect_snapshot(list_simplify(1:5), error = TRUE)
})
8 changes: 4 additions & 4 deletions tests/testthat/test-reduce.R
Original file line number Diff line number Diff line change
Expand Up @@ -162,8 +162,8 @@ test_that("basic accumulate2() works", {
paste2 <- function(x, y, sep) paste(x, y, sep = sep)

x <- c("a", "b", "c")
expect_equal(accumulate2(x, c("-", "."), paste2), list("a", "a-b", "a-b.c"))
expect_equal(accumulate2(x, c(".", "-", "."), paste2, .init = "x"), list("x", "x.a", "x.a-b", "x.a-b.c"))
expect_equal(accumulate2(x, c("-", "."), paste2), c("a", "a-b", "a-b.c"))
Copy link
Member

Choose a reason for hiding this comment

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

I wonder if the switch to auto simplification will break much code

Copy link
Member Author

Choose a reason for hiding this comment

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

There's only one use of accumulate2() on CRAN, so I don't think it's likely to affect much user code.

expect_equal(accumulate2(x, c(".", "-", "."), paste2, .init = "x"), c("x", "x.a", "x.a-b", "x.a-b.c"))
})

test_that("can terminate accumulate2() early", {
Expand All @@ -177,8 +177,8 @@ test_that("can terminate accumulate2() early", {
}

x <- c("a", "b", "c")
expect_equal(accumulate2(x, c("-", "."), paste2), list("a", "a-b"))
expect_equal(accumulate2(x, c(".", "-", "."), paste2, .init = "x"), list("x", "x.a", "x.a-b"))
expect_equal(accumulate2(x, c("-", "."), paste2), c("a", "a-b"))
expect_equal(accumulate2(x, c(".", "-", "."), paste2, .init = "x"), c("x", "x.a", "x.a-b"))
})

test_that("accumulate2() forces arguments (#643)", {
Expand Down
24 changes: 0 additions & 24 deletions tests/testthat/test-utils.R
Original file line number Diff line number Diff line change
Expand Up @@ -74,30 +74,6 @@ test_that("quo_invert() unwraps constants", {
expect_identical(quo_invert(call), new_quosure(quote(foo(foo, NULL)), quo_get_env(foo)))
})

test_that("vec_simplify() coerces atomic inputs", {
expect_identical(
vec_simplify(list(1, TRUE)),
c(1, 1)
)
expect_identical(
vec_simplify(list("foo", factor("bar"))),
c("foo", "bar")
)
expect_identical(
vec_simplify(list(data.frame(x = FALSE), data.frame(x = 1L))),
data.frame(x = 0:1)
)
})

test_that("vec_simplify() ignores complex inputs", {
expect_identical(vec_simplify(list(1L, 2:3)), list(1L, 2:3))
expect_identical(vec_simplify(list(1, "a")), list(1, "a"))
expect_identical(vec_simplify(1:3), 1:3)
expect_identical(vec_simplify(list(identity)), list(identity))
expect_identical(vec_simplify(mtcars), mtcars)
})


# Lifecycle ---------------------------------------------------------------

test_that("%@% is an infix attribute accessor", {
Expand Down