-
Notifications
You must be signed in to change notification settings - Fork 276
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
Improve list_simplify() performance + errors #942
Conversation
Oooh, and name management from https://github.com/tidyverse/purrr/pull/894/files#r957388812. |
R/list-simplify.R
Outdated
x <- vec_set_names(x, NULL) | ||
out <- list_unchop(x, ptype = ptype) |
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.
Hmm, I think this is slightly wrong because if the unchop fails and strict = FALSE
, then we will return x
which might have had its names stripped by this
Test for this case is probably useful, something like "returns a named list when input is named and simplification fails"
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.
Oops yes. Let me refactor.
R/list-simplify.R
Outdated
@@ -83,8 +101,12 @@ simplify_impl <- function(x, | |||
) | |||
} else { | |||
if (strict) { | |||
bad <- detect_index(x, function(x) vec_size(x) != 1) |
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.
You could also do
which(list_sizes(x) != 1L)[[1L]]
or store the result of list_sizes(x) == 1L
as where_size_one
from when you computed it above in the strict
branch along with the list_check_all_vectors()
check, and then reference it here like bad <- which(!where_size_one)[[1L]]
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 moved this error up into the earlier strict branch which I think makes it easier to follow.
Co-authored-by: Davis Vaughan <davis@rstudio.com>
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.
Looks great now!
Fixes #935