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

Argument matching in partial() doesn't happen for primitive functions #360

Closed
yutannihilation opened this issue Aug 4, 2017 · 9 comments

Comments

@yutannihilation
Copy link
Member

commented Aug 4, 2017

NEWS on version 0.2.3 says:

For instance + is transformed to function(.x, .y) .x + .y. This
results in proper argument matching so that map(1:10, partial(-, .x = 5)) produces list(5 - 1, 5 - 2, ...).

If I understand correctly, using .y instead of .x, map(1:10, partial(-, .y = 5)), will produce list(1 - 5, 2 - 5, ...), right? But it won't:

map_int(1:10, partial(`-`, .y = 5L))
#>  [1]  4  3  2  1  0 -1 -2 -3 -4 -5

Any argument will produce the same result. What does it mean by "proper argument matching" here? Should I use as_mapper() explicitly by myself?

map_int(1:10, partial(`-`, any_inproper_argument = 5L))
#>  [1]  4  3  2  1  0 -1 -2 -3 -4 -5
@egnha

This comment has been minimized.

Copy link
Contributor

commented Aug 4, 2017

Primitive functions, like -, are “special,” in that they don't generally have the match-by-name semantics for arguments that closures do. This is why the any_improper_argument case also gives the same result. Additionally, partial() will put the set values in front of the others, by default:

partial(`-`, .y = 5L)
#> function (...) 
#> 5L - ...

So you have to reverse that:

partial(`-`, .y = 5L, .first = FALSE)
#> function (...) 
#> ... - 5L
@yutannihilation

This comment has been minimized.

Copy link
Member Author

commented Aug 4, 2017

Thanks for the comment. Yes, I know it and the work around of using .first. But, NEWS says we can match by using as_mapper() (and I thought it is automatically done in partial()).

map_int(1:10, partial(as_mapper(`-`), .y = 5L))
@egnha

This comment has been minimized.

Copy link
Contributor

commented Aug 4, 2017

@yutannihilation Then you are right that the documentation is not completely accurate, and maybe even a bit misleading.

@yutannihilation

This comment has been minimized.

Copy link
Member Author

commented Aug 4, 2017

If this is merely a documentation problem, I'm happy to close this issue :)

@lionel-

This comment has been minimized.

Copy link
Member

commented Aug 7, 2017

It seems we forgot to call as_closure() in partial().

@yutannihilation

This comment has been minimized.

Copy link
Member Author

commented Aug 18, 2017

@lionel-

Are you going to fix this? I'm getting a feeling that this is rather ok with the current behaviour because automatically converting primitives with as_closure() is a bit tricky.

As the prototype says the arguments are e1 and e2, I naturally assumed the argument to pass to partial() is e1 or e2 when I attempted to use partial() with primitive functions at the first time. I mean, those who naively use partial() with primitive functions will get confused no matter whether as_quosure() is automatically applied or not.

`-`
#> function (e1, e2)  .Primitive("-")

I guess adding some notes about primitive functions on the documentation of partial() is enough.

@lionel-

This comment has been minimized.

Copy link
Member

commented Aug 18, 2017

Note that those argument names e1 and e2 are only indicative and are never used by the primitive function.

I thought it was a good idea to use .x and .y so you could do stuff like map(1:3, -, .x = 10), but it of course doesn't work because .x matches the map() argument :/.

@yutannihilation

This comment has been minimized.

Copy link
Member Author

commented Aug 18, 2017

Note that those argument names e1 and e2 are only indicative and are never used by the primitive function.

This is what I didn't know until attempting to match e2... 😇

.x matches the map() argument :/.

Oh, sad...

@lionel-

This comment has been minimized.

Copy link
Member

commented Nov 29, 2018

Binary operators wrapped with as_closure() now support both purrr-style and base-style arguments btw (you need dev rlang though).

lionel- added a commit to lionel-/lowliner that referenced this issue Dec 20, 2018

@lionel- lionel- closed this in #607 Dec 21, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants
You can’t perform that action at this time.