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

Function calls in functions can not access to environment variables in parent functions #386

Closed
psychelzh opened this issue Aug 15, 2022 · 9 comments · Fixed by #387
Closed
Labels
bug an unexpected problem or unintended behavior

Comments

@psychelzh
Copy link

@markfairbanks Still fails in such circumstances (seemingly, function calls in functions can not access environment variables, either):

library(dplyr)
#> 
#> Attaching package: 'dplyr'
#> The following objects are masked from 'package:stats':
#> 
#>     filter, lag
#> The following objects are masked from 'package:base':
#> 
#>     intersect, setdiff, setequal, union
library(dtplyr)
f <- function(x, y, .v = "x") {
  inner_join(
    mutate(x, a = .v),
    y,
    by = character()
  )
}
x <- data.frame(a = "y")
y <- data.frame(b = "x")
f(x, y)
#>   a b
#> 1 x x
lazy_dt(x) |> 
  f(y)
#> Error in .checkTypos(e, names_x): Object '.v' not found. Perhaps you intended .cross_join_col

Created on 2022-08-15 by the reprex package (v2.0.1)

Originally posted by @psychelzh in #317 (comment)

@markfairbanks markfairbanks added the bug an unexpected problem or unintended behavior label Aug 15, 2022
@markfairbanks
Copy link
Collaborator

markfairbanks commented Aug 15, 2022

Simpler reprex:

library(dplyr, w = FALSE)
library(dtplyr)

f <- function(x, .v = "x") {
  mutate(x, a = .v)
}

x <- data.frame(a = "y")

lazy_dt(x) |> 
  f()
#> Error in eval(jsub, SDenv, parent.frame()): object '.v' not found

@markfairbanks
Copy link
Collaborator

The issue - lazy_dt() marks the global environment as the evaluation environment. But in this case we need the evaluation environment to be the function environment of f() because that's where .v exists.

library(dplyr, w = FALSE)
library(dtplyr)

f <- function(x, .v = "x") {
  mutate(x, a = .v)
}

x <- lazy_dt(data.frame(a = "y"))

x$env
#> <environment: R_GlobalEnv>

@markfairbanks
Copy link
Collaborator

markfairbanks commented Aug 15, 2022

We could always grab the evaluation environment from the ... passed to mutate() (or filter/summarize/etc.) by capturing the dots as quosures and extracting the environment from them. This solves the original issue.

However this would fail if two functions (let's say a mutate() and a filter()) were each defined in separate function environments.

my_mutate <- function(x, .v = "x") {
  mutate(x, a = .v)
}

my_filter <- function(x, .val = "x") {
  filter(x, a == .val)
}

x <- lazy_dt(data.frame(a = "y"))

x %>%
  my_mutate() %>%
  my_filter()

I'm not too familiar with "combining" environments (and possible issues or performance implications), but as far as I can tell that would be the only way to deal with this.

@eutwt
Copy link
Collaborator

eutwt commented Aug 15, 2022

Until/unless this is fixed in dtplyr, a workaround is to inject the variable

library(dplyr, w = FALSE)
library(dtplyr)

f <- function(x, .v = "x") {
  mutate(x, a = !!.v)
}

x <- data.frame(a = "y")

lazy_dt(x) |> 
  f()
#> Source: local data table [1 x 1]
#> Call:   copy(`_DT1`)[, `:=`(a = "x")]
#> 
#>   a    
#>   <chr>
#> 1 x    
#> 
#> # Use as.data.table()/as.data.frame()/as_tibble() to access results

Created on 2022-08-15 by the reprex package (v2.0.1.9000)

@eutwt

This comment was marked as outdated.

@psychelzh
Copy link
Author

psychelzh commented Aug 15, 2022

Perhaps this bug is the same as #260.

@eutwt
Copy link
Collaborator

eutwt commented Aug 16, 2022

Ignore my previous comment. I think the issue is just that because .v starts with ., dtplyr treats it as a data.table symbol e.g. .N and doesn't do any injection.

} else if (nchar(x) > 0 && substr(var, 1, 1) == ".") {

@psychelzh
Copy link
Author

psychelzh commented Aug 16, 2022

Ignore my previous comment. I think the issue is just that because .v starts with ., dtplyr treats it as a data.table symbol e.g. .N and doesn't do any injection.

} else if (nchar(x) > 0 && substr(var, 1, 1) == ".") {

Yeah, you are correct!

library(dplyr, w = FALSE)
library(dtplyr)

f <- function(x, v = "x") {
  mutate(x, a = v)
}

x <- data.frame(a = "y")

lazy_dt(x) |>
  f()
#> Source: local data table [1 x 1]
#> Call:   copy(`_DT1`)[, `:=`(a = "x")]
#> 
#>   a    
#>   <chr>
#> 1 x    
#> 
#> # Use as.data.table()/as.data.frame()/as_tibble() to access results

Created on 2022-08-16 by the reprex package (v2.0.1)

So this won't be fixed? Users should inject it by themselves?

@eutwt
Copy link
Collaborator

eutwt commented Aug 16, 2022

I think we can just do a better job of detecting whether the variable is a data.table symbol like .N (e.g. .v is not), by checking against an actual list c(".SD", ".BY", ".N", ".I", ".GRP", ".NGRP")

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug an unexpected problem or unintended behavior
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants