WinVector / wrapr

Wrap R for Sweet R Code
https://winvector.github.io/wrapr/
Other
135 stars 11 forks source link

Some inconsistencies with `%.>%` and parens use #6

Closed moodymudskipper closed 5 years ago

moodymudskipper commented 5 years ago

in the couple of cases below wrapr is not consistent where alternatives are.

library(magrittr)
library(pipeR)
library(wrapr)

1:5 %>% mean
#> [1] 3
1:5 %.>% mean
#> [1] 3
1:5 %>>% mean
#> [1] 3

mean <- "foo"

1:5 %>% mean
#> [1] 3
1:5 %.>% mean
#> Error: wrapr::apply_right_S4 default called with classes:
#>   integer 
#>  mean character 
#>   must have a more specific S4 method defined to dispatch
1:5 %>>% mean
#> [1] 3

1:5 %>% mean()
#> [1] 3
1:5 %.>% mean(.)
#> [1] 3
1:5 %>>% mean()
#> [1] 3

test %>% substitute
#> Error in eval(lhs, parent, parent): objet 'test' introuvable
test %.>% substitute
#> test
test %>>% substitute
#> Error in test %>>% substitute: objet 'test' introuvable

`%.%` <- function(e1,e2){
  eval.parent(eval(substitute(substitute(e2,list(. = substitute(e1))))))
}
test %>% substitute(.)
#> Error in eval(lhs, parent, parent): objet 'test' introuvable
test %.>% substitute(.)
#> Error in eval(pipe_left_arg, envir = pipe_environment, enclos = pipe_environment): objet 'test' introuvable
test %>>% substitute(.)
#> Error in test %>>% substitute(.): objet 'test' introuvable
test %.% substitute(.)
#> test

Created on 2018-12-16 by the reprex package (v0.2.0).

JohnMount commented 5 years ago

First thanks for the insightful note, and sorry if the wrapr pipe is causing you any trouble. I use it in my work all the time, so it is becoming second nature for me.

This response is going to be long, as I want to try and use your examples as an opportunity to clarify some wrapr dot-pipe intent. I also have an older study on wrapr versus magtittr pipe consistency here. I agree it is wrong when systems don't meet expectations; however I want to spend a bit of time trying to refine expectations.

In the cases you are showing wrapr pipe is consistent with inferences from its documentation.

Roughly wrapr dot-pipe's primary semantics are:

For the first problem case:

library("wrapr")
mean <- "foo"
1:5 %.>% mean
# Error: wrapr::apply_right_S4 default called with classes:
#  integer 
# mean character 
#  must have a more specific S4 method defined to dispatch 

This is one of the reasons that 1:5 %.>% mean(.) is the preferred notation (name lookup is an add-on from wrapr pipe's point of view).

The issue is: wrapr can pipe into objects (which can specify functional behavior) in addition to functions and expressions. To support this wrapr doesn't use separate name-spaces for functions and other objects. The above is a minus, but the design is what allows class controlled right dispatch for packages such as rquery (or even the SQLite example in the documentation). The error message is an attempt to show the user what happened (the environment value of mean, which is character was use- not the package-base value which is a function).

An artificial (though not useful) example of piping into objects is:

library("wrapr")

setMethod(
  "apply_right_S4",
  signature(pipe_left_arg = "character", pipe_right_arg = "character"),
  function(pipe_left_arg,
           pipe_right_arg,
           pipe_environment,
           left_arg_name,
           pipe_string,
           right_arg_name) {
    return(paste(as.character(pipe_left_arg), pipe_right_arg))
  })

b <- "bstr"
"a" %.>% b
# [1] "a bstr"

# This intentionally does not allow "a" %.>% "b"

Roughly: piping into a value is less powerful than piping into a name or expression in wrapr.

However the rquery package and the wrapr package both use these distinctions for a number of beneficial effects (the wrapr pipe underlies the effects here, though it is not called out).

For the last problem case:

library("wrapr")
test %.>% substitute(.)

None of the package-pipes did well (so they were consistent in that sense). wrapr pipe documentation does admit to evaluating the left argument first, so the failure is consistent with the documentation. Also, from wrapr's point of view subtitute(.) is being called with an argument named ".", wrapr only preserves names in the 5 %.>% substitute case.

The proposed dot-only pipe is definitely interesting (and I don't mind a pipe insisting on dot). But it is a different pipe than the wrapr one. If you want to explore it and write a note on it I'd be happy write a favorable note/link in the Win-Vector blog. My email is jmount@win-vector.com and I would be happy to hear more from you.

moodymudskipper commented 5 years ago

Hi John, apologies for answering so late, especially as you always respond so promptly. I think I was busy and just forgot about it.

Thanks for your insightful response, it's interesting how something so simple in appearance as the pipe can be implemented in so many different ways, with different expectations.

I'm usually satisfied with magrittr's pipe and I'd probably be satisfied with any implementation as I believe whenever I'm doing something a bit more complex I'm better off writing it without pipes for clarity, but magrittr's implementation is really strange, relying on detecting symbols...

I designed a package pipes which builds on magrittr, making pipes an object with a given class and printing method, so I had to look into the code and I was puzzled by the complexity (I haven't look at yours or Kun Ren' implementations though to be honest), so it got me thinking about corner cases and I investigated some more.

I never intended to use in practice the pipe that I designed above, it solves one corner case, but it will substitute all dots in the rhs , even in formulas, and each dots on the rhs will be recomputed, so it's both less efficient and less practical (this will not give the expected result: list(1,2) %.% purrr::map(., ~identity(.))) ).

Here are some more edge cases which I think make wrapr look good (appart from a bug, . is changed in global environement when using function operators, see below)

library(magrittr)
library(wrapr)
#> Warning: package 'wrapr' was built under R version 3.5.3
library(pipeR)
#> Warning: package 'pipeR' was built under R version 3.5.3
length(length)
#> [1] 1

create_dim_from_anything <- function(...) {
  print(list(...))
  dim
}
iris2 <- head(iris,2)
. <- length
iris2 %>% create_dim_from_anything()() # dot is inserted in 2nd () only, as expected
#> list()
#> [1] 2 5
iris2 %>% create_dim_from_anything(.)(.) # I expected substitution of 1st dot by `length`
#> [[1]]
#>   Sepal.Length Sepal.Width Petal.Length Petal.Width Species
#> 1          5.1         3.5          1.4         0.2  setosa
#> 2          4.9         3.0          1.4         0.2  setosa
#> [1] 2 5

iris2 %.>% create_dim_from_anything()(.)  #  expected, but it changed the value of . !!!
#> list()
#> [1] 2 5
.
#>   Sepal.Length Sepal.Width Petal.Length Petal.Width Species
#> 1          5.1         3.5          1.4         0.2  setosa
#> 2          4.9         3.0          1.4         0.2  setosa
. <- length
iris2 %.>% create_dim_from_anything(.)(.) # I expected substitution of 1st dot by `length`
#> [[1]]
#>   Sepal.Length Sepal.Width Petal.Length Petal.Width Species
#> 1          5.1         3.5          1.4         0.2  setosa
#> 2          4.9         3.0          1.4         0.2  setosa
#> [1] 2 5
. <- length

iris2 %>>% create_dim_from_anything()() # dot is inserted in 2nd () only, as expected
#> list()
#> [1] 2 5
iris2 %>>% create_dim_from_anything(.)() # I expected substitution of 1st dot by `length`
#> [[1]]
#>   Sepal.Length Sepal.Width Petal.Length Petal.Width Species
#> 1          5.1         3.5          1.4         0.2  setosa
#> 2          4.9         3.0          1.4         0.2  setosa
#> [1] 2 5

is.function %>% .     # unexpected, I expected length(is.function)
#> [1] TRUE
is.function %>% .()   # unexpected but consistent with previous
#> [1] TRUE
is.function %>% .(.)  # unexpected but consistent with previous
#> [1] TRUE
is.function %>% (.)   # expected
#> [1] 1
is.function %>% (.)() # unexpected and very insonsitent with previous
#> [1] TRUE
is.function %>% {.}   # expected
#> function (x)  .Primitive("is.function")
is.function %>% {.}() # expected
#> [1] TRUE
is.function %>% {.}(.) # expected
#> [1] TRUE

is.function %.>% .      # unexpected but helpful error 
#> Error in pipe_impl(pipe_left_arg, pipe_right_arg, pipe_environment, pipe_string): to reduce surprising behavior wrapr::pipe does not allow direct piping into some names, such as .
is.function %.>% .(.)   # expected
#> [1] 1
is.function %.>% (.)    # unexpected but helpful error 
#> Error in pipe_impl(pipe_left_arg, pipe_right_arg, pipe_environment, pipe_string): to reduce surprising behavior wrapr::pipe does not allow direct piping into some names, such as .
is.function %.>% (.)(.) # expected
#> [1] TRUE
. <- length
is.function %.>% {.}    # expected
#> function (x)  .Primitive("is.function")
is.function %.>% {.}(.) # expected
#> [1] TRUE
. <- length

is.function %>>% .    # unexpected, I expected length(is.function)
#> [1] TRUE
is.function %>>% .()  # unexpected but consistent with previous
#> [1] TRUE
is.function %>>% (.)   # unexpected failure
#> Error in object[[name, exact = TRUE]]: object of type 'builtin' is not subsettable
is.function %>>% (.)() # [1] TRUE, unexpected considering previous
#> [1] TRUE
is.function %>>% {.}   # expected
#> function (x)  .Primitive("is.function")
is.function %>>% {.}() # expected
#> [1] TRUE

Created on 2019-04-27 by the reprex package (v0.2.1)

moodymudskipper commented 5 years ago

please see update, the reprex made me realised there's a bug in wrapr modyfing . in the global environment

JohnMount commented 5 years ago

Thanks for looking into that and sorry about any confusion.

wrapr pipe deliberately executes in the environment it is dispatched in (not in an new intermediate environment) to try and not lose any other side effects the target expression may want to form. So altering dot in the execution environment may or may not be undesirable, but it is by design.

Basically (for right or wrong) wrapr pipe considers dot to belong to it. Your examples are showing a more shared view of dot.