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

Rework simplification #909

merged 38 commits into from
Sep 12, 2022

Conversation

hadley
Copy link
Member

@hadley hadley commented Sep 2, 2022

With this, map_vec() will become:

map_vec <- function(.x, .f, ..., .ptype = NULL) {
   out <- unname(map(.x, .f, ...))
   out <- list_simplify(out ,f. simplify = TRUE, ptype = .ptype)
   set_names(out, names(x))
}

R/list-simplify.R Outdated Show resolved Hide resolved
Copy link
Member

@lionel- lionel- left a comment

Choose a reason for hiding this comment

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

Errors are missing full stops.

R/list-simplify.R Show resolved Hide resolved
R/list-simplify.R Outdated Show resolved Hide resolved
R/list-simplify.R Outdated Show resolved Hide resolved
@hadley hadley changed the title Explore possible list_simplify() implementation Rework simplification Sep 4, 2022
@hadley hadley marked this pull request as ready for review September 9, 2022 14:24
R/list-simplify.R Outdated Show resolved Hide resolved
R/list-simplify.R Outdated Show resolved Hide resolved
R/list-simplify.R Outdated Show resolved Hide resolved
R/list-simplify.R Outdated Show resolved Hide resolved
R/list-simplify.R Outdated Show resolved Hide resolved
R/list-transpose.R Outdated Show resolved Hide resolved
R/list-transpose.R Outdated Show resolved Hide resolved
R/list-transpose.R Show resolved Hide resolved
tests/testthat/_snaps/reduce.md Outdated Show resolved Hide resolved
tests/testthat/test-list-transpose.R Outdated Show resolved Hide resolved
hadley and others added 2 commits September 12, 2022 09:41
Co-authored-by: Lionel Henry <lionel.hry@gmail.com>
@hadley hadley requested a review from DavisVaughan September 12, 2022 19:01
R/coercion.R Outdated Show resolved Hide resolved
R/coercion.R Outdated Show resolved Hide resolved
Comment on lines 3 to 7
#' @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()].
Copy link
Member

Choose a reason for hiding this comment

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

No @description? I feel like these details would probably make a decent description instead

Copy link
Member Author

Choose a reason for hiding this comment

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

Oops. No idea why I did that.

R/list-simplify.R Show resolved Hide resolved
#' 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)
Copy link
Member

Choose a reason for hiding this comment

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

Maybe do some validation of strict?

error_call = caller_env()
) {
if (length(simplify) > 1 || !is.logical(simplify)) {
cli::cli_abort("{.arg simplify} must be `TRUE`, `FALSE`, or `NA`.", arg = "simplify")
Copy link
Member

Choose a reason for hiding this comment

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

Pass error_call through

cli::cli_abort("{.arg simplify} must be `TRUE`, `FALSE`, or `NA`.", arg = "simplify")
}
if (!is.null(ptype) && isFALSE(simplify)) {
cli::cli_abort("Can't specify {.arg ptype} when `simplify = FALSE`.")
Copy link
Member

Choose a reason for hiding this comment

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

Pass error_call through

Copy link
Member

Choose a reason for hiding this comment

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

Should arg = "ptype" be set?

@@ -162,8 +152,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.

Comment on lines +44 to +45
* `transpose()` has been deprecated in favour of `list_transpose()` (#875).
It has built-in simplification.
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 my biggest worry is that list_transpose() is probably a lot slower than transpose(), but I guess it is way more featureful and generic

Copy link
Member Author

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.

R/list-simplify.R Outdated Show resolved Hide resolved
@hadley hadley merged commit 1276623 into main Sep 12, 2022
@hadley hadley deleted the simplify branch September 12, 2022 22:22
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
3 participants