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

Issue of lazy evaluation with dev magrittr #4

Closed
lionel- opened this issue Aug 25, 2020 · 4 comments
Closed

Issue of lazy evaluation with dev magrittr #4

lionel- opened this issue Aug 25, 2020 · 4 comments

Comments

@lionel-
Copy link

lionel- commented Aug 25, 2020

Hello, I'm seeing an issue with the development version of magrittr which switches to a lazy evaluation approach. It causes this error in your package:

> aframe <- data.frame(zed = runif(100))
> set_to_zero <- . %>% mutate(zed = 0)
> aframe %>% scion(zed >0.5, false_fun=set_to_zero) %>% mutate(zed=1) %>% graft
Error in .pop() : 
  No more scions on stack. Perhaps you meant to specify the "data2" argument to graft()?
Calls: %>% -> graft -> .pop

That's because the pipeline is now evaluated as if it were nested, instead of linearly:

graft(mutate(scion(aframe, zed > 0.5, false_fun = set_to_zero), zed = 1))

Because of lazy evaluation, the input to graft has not been evaluated yet when you check if data2 is missing. One way to fix this is to force() the input like this:

--- a/R/tabe.R
+++ b/R/tabe.R
@@ -62,6 +62,7 @@ scion <- function(.data, ..., false_fun, false_name, false_env){
 #' @importFrom dplyr intersect
 #' @importFrom dplyr full_join
 graft <- function(.data, combine_fun, data2){
+  force(.data)
   if(missing(data2))
     data2 <- .pop()

We're planning to release magrittr in one month. It'd be great if you could update your package :)

Note that the future native pipe in the next version of R will use this same approach, so by fixing your package for the new magrittr behaviour you'll make it usable with the native pipe as well.

@restonslacker
Copy link
Owner

Thank you for the report and the suggested fix. I will work on this week. it's been quite a while since i've worked on this package so i suspect it will need some other updates to remain in compliance with CRAN.

@restonslacker
Copy link
Owner

package submitted to CRAN. awaiting response.

@restonslacker
Copy link
Owner

accepted on cran

@lionel-
Copy link
Author

lionel- commented Oct 6, 2020

Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants