-
Notifications
You must be signed in to change notification settings - Fork 275
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
Changes from 32 commits
bcc5415
1ead93b
a927496
be3fe11
17bffab
8d63020
02ea731
71ba50c
20387d0
7805591
5d6d874
d9c76d3
1980e24
eb85559
5be021d
5940cc2
675a8a8
8b32bfa
a4f864d
822d714
5254896
c8b4bc9
e4311be
80fa90c
41b2039
a7a41c8
4577473
5319949
fcb92d6
2397f21
471f340
b16071a
da1f7c9
0590ccf
99c925e
808568b
2c5866a
78938f7
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,77 @@ | ||
#' Simplify a list to an atomic or S3 vector | ||
#' | ||
#' @details | ||
#' Simplification maintains a one-to-one correspondence between the input | ||
#' and output, implying that each element of `x` must contain a vector of | ||
#' length 1. If you don't want to maintain this correspondence, then you | ||
#' probably want either [list_c()] or [list_flatten()]. | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. No There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Oops. No idea why I did that. |
||
#' | ||
#' @param x A list. | ||
#' @param strict What should happen if simplification fails? If `TRUE`, | ||
#' will error. If `FALSE`, will return `x` unchanged. | ||
#' @param ptype An optional prototype to ensure that the output type is always | ||
#' the same. | ||
hadley marked this conversation as resolved.
Show resolved
Hide resolved
|
||
#' @returns A vector the same length as `x`. | ||
#' @export | ||
#' @examples | ||
#' list_simplify(list(1, 2, 3)) | ||
#' | ||
#' try(list_simplify(list(1, 2, "x"))) | ||
#' try(list_simplify(list(1, 2, 1:3))) | ||
list_simplify <- function(x, strict = TRUE, ptype = NULL) { | ||
simplify_impl(x, strict = strict, ptype = ptype) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Maybe do some validation of |
||
} | ||
|
||
# Wrapper used by purrr functions that do automatic simplification | ||
list_simplify_internal <- function(x, | ||
simplify = NA, | ||
ptype = NULL, | ||
error_call = caller_env() | ||
) { | ||
hadley marked this conversation as resolved.
Show resolved
Hide resolved
|
||
if (length(simplify) > 1 || !is.logical(simplify)) { | ||
cli::cli_abort("{.arg simplify} must be `TRUE`, `FALSE`, or `NA`.", arg = "simplify") | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Pass |
||
} | ||
if (!is.null(ptype) && isFALSE(simplify)) { | ||
cli::cli_abort("Can't specify {.arg ptype} when `simplify = FALSE`.") | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Pass There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Should |
||
} | ||
|
||
if (isFALSE(simplify)) { | ||
hadley marked this conversation as resolved.
Show resolved
Hide resolved
|
||
return(x) | ||
} | ||
|
||
simplify_impl( | ||
x, | ||
strict = !is.na(simplify), | ||
ptype = ptype, | ||
error_call = error_call | ||
) | ||
} | ||
|
||
simplify_impl <- function(x, | ||
strict = TRUE, | ||
ptype = NULL, | ||
error_call = caller_env()) { | ||
vec_check_list(x, call = error_call) | ||
|
||
can_simplify <- every(x, vec_is, size = 1) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. How do we feel about list_simplify(list(lm(1 ~ 1)), strict = FALSE) I feel like this should still error, because purrr functions should only work on vector types? I feel like
i.e. this scalar object issue is out of scope and would still be an error If you agree, then I'd argue that the current implementation here will be very slow ( list_check_all_vectors(x, call = error_call)
can_simplify <- all(list_sizes(x) == 1L) There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Even if you want to support scalar types, it might be worth using the approach above and wrapping it in |
||
|
||
if (can_simplify) { | ||
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 | ||
} | ||
} | ||
) | ||
} else { | ||
if (strict) { | ||
cli::cli_abort("All elements must be length-1 vectors.", call = error_call) | ||
} else { | ||
x | ||
} | ||
} | ||
} |
Original file line number | Diff line number | Diff line change | ||||||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
@@ -0,0 +1,127 @@ | ||||||||||||||||||||||
#' Transpose a list | ||||||||||||||||||||||
#' | ||||||||||||||||||||||
#' @description | ||||||||||||||||||||||
#' `list_transpose()` turns a list-of-lists "inside-out". For instance it turns a pair of | ||||||||||||||||||||||
#' lists into a list of pairs, or a list of pairs into pair of lists. For | ||||||||||||||||||||||
hadley marked this conversation as resolved.
Show resolved
Hide resolved
|
||||||||||||||||||||||
#' example, if you had a list of length `n` where each component had values `a` | ||||||||||||||||||||||
#' and `b`, `list_transpose()` would make a list with elements `a` and | ||||||||||||||||||||||
#' `b` that contained lists of length n. | ||||||||||||||||||||||
hadley marked this conversation as resolved.
Show resolved
Hide resolved
|
||||||||||||||||||||||
#' | ||||||||||||||||||||||
#' It's called transpose because `x[["a"]][["b"]]` is equivalent to | ||||||||||||||||||||||
#' `transpose(x)[["b"]][["a"]]`, i.e. transposing a list flips the order of | ||||||||||||||||||||||
hadley marked this conversation as resolved.
Show resolved
Hide resolved
|
||||||||||||||||||||||
#' indices in a similar way to transposing a matrix. | ||||||||||||||||||||||
#' | ||||||||||||||||||||||
#' @param x A list of vectors to transpose. | ||||||||||||||||||||||
#' @param template A "template" that specifies the names of output list. | ||||||||||||||||||||||
hadley marked this conversation as resolved.
Show resolved
Hide resolved
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It actually isn't clear to me that this is supposed to be a character vector. It seems like it is supposed to be a named vector where the names get used as the output names, like Is there any reason not to call it There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Oops forgot to update these docs. Hopefully it's more obvious why it's called |
||||||||||||||||||||||
#' Usually taken from the name of the first element of `x`. | ||||||||||||||||||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think it is very useful to mention that Like, I didn't know that ll <- list(
list(x = 1, y = "one"),
list(z = "deux", x = 2)
)
ll %>% list_transpose(template = 1) |
||||||||||||||||||||||
#' @param simplify Should the result be [simplified][list_simplify]? | ||||||||||||||||||||||
#' * `TRUE`: simplify or die trying. | ||||||||||||||||||||||
#' * `NA`: simplify if possible. | ||||||||||||||||||||||
#' * `FALSE`: never try to simplify, always leaving as a list. | ||||||||||||||||||||||
#' | ||||||||||||||||||||||
#' Alternatively, a named list specifying the simplification by output column. | ||||||||||||||||||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. You say |
||||||||||||||||||||||
#' @param ptype An optional vector prototype used to control the simplification. | ||||||||||||||||||||||
#' Alternatively, a named list specifying the prototype by output column. | ||||||||||||||||||||||
#' @param default A default value to use if a value is absent of `NULL`. | ||||||||||||||||||||||
hadley marked this conversation as resolved.
Show resolved
Hide resolved
|
||||||||||||||||||||||
#' Alternatively, a named list specifying the prototype by output column. | ||||||||||||||||||||||
hadley marked this conversation as resolved.
Show resolved
Hide resolved
|
||||||||||||||||||||||
#' @export | ||||||||||||||||||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Random comment: I tried this somewhere along the way while exploring this and this error didn't make much sense to me x <- list(
a = list(integer(), "x"),
b = list(2L, "y")
)
list_transpose(x, default = list(a = NA))
#> Error in `match_template()` at purrr/R/list-transpose.R:76:2:
#> ! List `default` must be same length as numeric template
Also it looks like a There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Now: Length of |
||||||||||||||||||||||
#' @examples | ||||||||||||||||||||||
#' # list_transpose() is useful in conjunction with safely() | ||||||||||||||||||||||
#' x <- list("a", 1, 2) | ||||||||||||||||||||||
#' y <- x %>% map(safely(log)) | ||||||||||||||||||||||
#' y %>% str() | ||||||||||||||||||||||
#' # Put all the errors and results together | ||||||||||||||||||||||
#' y %>% list_transpose() %>% str() | ||||||||||||||||||||||
#' # Supply a default result to further simplify | ||||||||||||||||||||||
#' y %>% list_transpose(default = list(result = NA)) %>% str() | ||||||||||||||||||||||
#' | ||||||||||||||||||||||
#' # list_transpose() 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() | ||||||||||||||||||||||
#' # this makes list_tranpose() not completely symmetric | ||||||||||||||||||||||
#' x %>% list_transpose() %>% list_transpose() | ||||||||||||||||||||||
#' | ||||||||||||||||||||||
#' # use simplify = FALSE to always return lists: | ||||||||||||||||||||||
#' x %>% list_transpose(simplify = FALSE) %>% str() | ||||||||||||||||||||||
hadley marked this conversation as resolved.
Show resolved
Hide resolved
|
||||||||||||||||||||||
#' x %>% | ||||||||||||||||||||||
#' list_transpose(simplify = FALSE) %>% | ||||||||||||||||||||||
#' list_transpose(simplify = FALSE) %>% str() | ||||||||||||||||||||||
#' | ||||||||||||||||||||||
#' # Provide explicit template if you know which elements you want to extract | ||||||||||||||||||||||
hadley marked this conversation as resolved.
Show resolved
Hide resolved
|
||||||||||||||||||||||
#' ll <- list( | ||||||||||||||||||||||
#' list(x = 1, y = "one"), | ||||||||||||||||||||||
#' list(z = "deux", x = 2) | ||||||||||||||||||||||
#' ) | ||||||||||||||||||||||
#' ll %>% list_transpose() | ||||||||||||||||||||||
#' ll %>% list_transpose(template = c("x", "y", "z")) | ||||||||||||||||||||||
#' ll %>% list_transpose(template = 1) | ||||||||||||||||||||||
#' | ||||||||||||||||||||||
#' # 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_check_list(x) | ||||||||||||||||||||||
if (length(x) == 0) { | ||||||||||||||||||||||
return(list()) | ||||||||||||||||||||||
} | ||||||||||||||||||||||
|
||||||||||||||||||||||
template <- template %||% vec_index(x[[1]]) | ||||||||||||||||||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
I'd love to avoid the early exit. It seems like the main problem is There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think this is a bit simpler:
|
||||||||||||||||||||||
if (!is.character(template) && !is.numeric(template)) { | ||||||||||||||||||||||
cli::cli_abort( | ||||||||||||||||||||||
"{.arg template} must be a character or numeric vector, not {.obj_type_friendly {template}}.", | ||||||||||||||||||||||
arg = template | ||||||||||||||||||||||
) | ||||||||||||||||||||||
} | ||||||||||||||||||||||
|
||||||||||||||||||||||
simplify <- match_template(simplify, template) | ||||||||||||||||||||||
default <- match_template(default, template) | ||||||||||||||||||||||
ptype <- match_template(ptype, template) | ||||||||||||||||||||||
|
||||||||||||||||||||||
out <- rep_along(template, list()) | ||||||||||||||||||||||
if (is.character(template)) { | ||||||||||||||||||||||
names(out) <- template | ||||||||||||||||||||||
} | ||||||||||||||||||||||
|
||||||||||||||||||||||
for (i in seq_along(template)) { | ||||||||||||||||||||||
idx <- template[[i]] | ||||||||||||||||||||||
res <- map(x, idx, .default = default[[i]]) | ||||||||||||||||||||||
res <- list_simplify_internal(res, | ||||||||||||||||||||||
simplify = simplify[[i]] %||% NA, | ||||||||||||||||||||||
ptype = ptype[[i]] | ||||||||||||||||||||||
) | ||||||||||||||||||||||
out[[i]] <- res | ||||||||||||||||||||||
} | ||||||||||||||||||||||
|
||||||||||||||||||||||
out | ||||||||||||||||||||||
} | ||||||||||||||||||||||
|
||||||||||||||||||||||
match_template <- function(x, template, error_arg = caller_arg(x), error_call = caller_env()) { | ||||||||||||||||||||||
if (is.character(template)) { | ||||||||||||||||||||||
if (is_bare_list(x) && is_named(x)) { | ||||||||||||||||||||||
extra_names <- setdiff(names(x), template) | ||||||||||||||||||||||
if (length(extra_names)) { | ||||||||||||||||||||||
cli::cli_abort( | ||||||||||||||||||||||
"{.arg {error_arg}} contains unknown names: {.str {extra_names}}", | ||||||||||||||||||||||
hadley marked this conversation as resolved.
Show resolved
Hide resolved
|
||||||||||||||||||||||
arg = error_arg, | ||||||||||||||||||||||
call = error_call | ||||||||||||||||||||||
) | ||||||||||||||||||||||
} | ||||||||||||||||||||||
|
||||||||||||||||||||||
out <- rep_named(template, list(NULL)) | ||||||||||||||||||||||
out[names(x)] <- x | ||||||||||||||||||||||
out | ||||||||||||||||||||||
} else { | ||||||||||||||||||||||
rep_named(template, list(x)) | ||||||||||||||||||||||
} | ||||||||||||||||||||||
} else if (is.numeric(template)) { | ||||||||||||||||||||||
if (is_bare_list(x) && length(x) > 0) { | ||||||||||||||||||||||
if (length(x) != length(template)) { | ||||||||||||||||||||||
cli::cli_abort("List {.arg {error_arg}} must be same length as numeric template") | ||||||||||||||||||||||
hadley marked this conversation as resolved.
Show resolved
Hide resolved
|
||||||||||||||||||||||
} | ||||||||||||||||||||||
x | ||||||||||||||||||||||
} else { | ||||||||||||||||||||||
rep_along(template, list(x)) | ||||||||||||||||||||||
} | ||||||||||||||||||||||
} else { | ||||||||||||||||||||||
abort("Invalid x", .internal = TRUE) | ||||||||||||||||||||||
hadley marked this conversation as resolved.
Show resolved
Hide resolved
|
||||||||||||||||||||||
} | ||||||||||||||||||||||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -56,7 +56,7 @@ | |
#' # now | ||
#' map2(arg1, arg2, ex_fun) %>% list_cbind() | ||
map_dfr <- function(.x, .f, ..., .id = NULL) { | ||
lifecycle::deprecate_warn("0.4.0", "map_dfr()", I("`map()` + `list_rbind()`")) | ||
lifecycle::deprecate_warn("0.4.0", "map_dfr()", I("`map()` + `list_rbind()`"), always = TRUE) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Not sure if this was meant to be here (i know itll get changed anyways) There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Ooops |
||
check_installed("dplyr", "for `map_dfr()`.") | ||
|
||
.f <- as_mapper(.f, ...) | ||
|
Original file line number | Diff line number | Diff line change | ||||
---|---|---|---|---|---|---|
|
@@ -342,11 +342,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 | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It also works if |
||||||
#' 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 | ||||||
|
@@ -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_internal(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_internal(res, .simplify, .ptype) | ||||||
res | ||||||
} | ||||||
|
||||||
accumulate_names <- function(nms, init, dir) { | ||||||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think my biggest worry is that
list_transpose()
is probably a lot slower thantranspose()
, but I guess it is way more featureful and genericThere was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, hopefully it's not used in too many performance critical places.