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.

Sign up for GitHub

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

Jump to bottom

... are further delayed in top_across() #5815

Merged
merged 10 commits into from
Apr 7, 2021
Merged

... are further delayed in top_across() #5815

merged 10 commits into from
Apr 7, 2021

Conversation

romainfrancois
Copy link
Member

closes #5813

library(dplyr, warn.conflicts = FALSE)

tibble(x = tibble(foo = 1)) %>%
  mutate(across(
    everything(),
    mutate, 
    foo = foo + 1
  ))
#> # A tibble: 1 x 1
#>   x$foo
#>   <dbl>
#> 1     2

Created on 2021-03-11 by the reprex package (v0.3.0)

@romainfrancois romainfrancois requested a review from lionel- March 11, 2021 10:10
@romainfrancois romainfrancois added this to the 1.0.6 milestone Mar 11, 2021
lionel-
lionel- reviewed Mar 11, 2021
View reviewed changes
R/across.R Outdated Show resolved Hide resolved
@lionel- lionel- force-pushed the top_across_delay_dots branch 2 times, most recently from 1661c4c to ca4d651 Compare March 22, 2021 13:22
This was referenced Mar 23, 2021
Closed
Merged
@lionel- lionel- force-pushed the top_across_delay_dots branch from ca4d651 to d4fd1e0 Compare March 23, 2021 15:56
@lionel- lionel- mentioned this pull request Mar 23, 2021
Merged
@lionel- lionel- force-pushed the top_across_delay_dots branch 3 times, most recently from 92c3668 to 9f724b5 Compare March 25, 2021 12:05
@romainfrancois romainfrancois mentioned this pull request Mar 25, 2021
Closed
17 tasks
romainfrancois and others added 6 commits April 2, 2021 13:17
@romainfrancois @lionel-
Delay ... in top_across() calls
c5e04c4
@lionel-
Fix semantics of ... within top-level across() calls
136ca6b
@lionel-
Use local dots to simplify logic
56dab56
@lionel-
Expand across() calls via match.call()
7ece00f 
Avoid evaluating `top_across()` in a data mask because this promises
the expressions in `...` within a different context than their
evaluation. Since quosure evaluation trigger a data mask rechain, the
data mask is not necessarily still chained to the caller environment
when the dots are eventually evaluated.
@lionel-
Fix CMD check note about global variable
0443b28
@lionel-
Update rlang remote
316fe17
@lionel- lionel- force-pushed the top_across_delay_dots branch from 9f724b5 to ff6aaf3 Compare April 2, 2021 14:44
@lionel- lionel- force-pushed the top_across_delay_dots branch from ff6aaf3 to 67d524e Compare April 2, 2021 15:01
@lionel-
Copy link
Member

lionel- commented Apr 2, 2021

Ready for another round of review @romainfrancois

romainfrancois
romainfrancois commented Apr 6, 2021
View reviewed changes
Copy link
Member Author

@romainfrancois romainfrancois left a comment

Choose a reason for hiding this comment

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

Thanks @lionel-. This looks great.

Eventually we might have more than expand_across(), i.e. other things to do with summarise(...=) but in the meantime, this looks great.

R/across.R
#' # Inside `across()`: 6 normal variates (ncol * ngroup)
#' gdf %>% mutate(across(v1:v2, ~ .x + rnorm(1)))
#' ````
#'
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, that reads great.

R/across.R
@@ -339,26 +386,75 @@ key_deparse <- function(key) {
# list(mean_x = expr(mean(x)), mean_y = expr(mean(y)))
# columns = c("x", "y")
# )
top_across <- function(.cols = everything(), .fns = NULL, ..., .names = NULL) {

expand_across <- function(quo) {
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 that's a better name :-)

R/across.R
# Expand dots in lexical env
env <- quo_get_env(quo)
expr <- match.call(
definition = across,
Copy link
Member Author

Choose a reason for hiding this comment

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

Nice. I didn't realize you could definition=

R/mutate.R
@@ -256,7 +256,7 @@ mutate_cols <- function(.data, ..., caller_env) {

# get results from all the quosures that are expanded from ..i
# then ingest them after
quosures <- expand_quosure(dots[[i]])
quosures <- expand_across(dots[[i]])
Copy link
Member Author

Choose a reason for hiding this comment

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

At some point we might want to do more than "expand across" here, e.g. perhaps we can expand if_any/all() also, and generally have some sort of mechanism for setup work, so we might add another layer, but for the moment this is more readable.

Copy link
Member

Choose a reason for hiding this comment

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

yup that makes sense.

@lionel- lionel- merged commit d07adb3 into master Apr 7, 2021
@lionel- lionel- deleted the top_across_delay_dots branch April 7, 2021 04:52
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@lionel- lionel- lionel- left review comments

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
1.0.6
Development

Successfully merging this pull request may close these issues.

... of across() are evaluated too early?
2 participants
@romainfrancois @lionel-