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

Thinking more about the interface of `invoke()` #132

Closed
lionel- opened this issue Nov 13, 2015 · 11 comments

Comments

@lionel-
Copy link
Member

commented Nov 13, 2015

I find the interface of invoke() a bit weird to use. I'd find it much more natural if both invoke() and invoke_map() received .x as first argument. It's generally this argument that gets manipulated in a
pipeline and I think .f does not have a different status than in other mapping tools.

Also, I'm still bugged about this:

a <- mtcars %>% map_n(paste)
b <- mtcars %>% transpose %>% invoke_map("paste", .x = .)

identical(a, b)
#> TRUE

The more I think about it, the more I feel map_n() has a different structure than map() and map2(). To me, it feels more like a variant of invoke().

Would it make sense to rename map_n() to invoke_zip() or invoke_map_t()? The zip suffix could be interpreted in purrr as mapping a transpose. Also we'd rename map_rows() to invoke_rows() (the transpose operation being implicit in choosing the rows, we don't need a t or zip particle here).

@lionel-

This comment has been minimized.

Copy link
Member Author

commented Nov 13, 2015

Would it make sense to rename map_n()

Or offer an alias to reflect the different point of views.

@hadley

This comment has been minimized.

Copy link
Member

commented Nov 13, 2015

I was thinking that maybe .x should be the first arg of invoke_*() too, but it looks a bit ugly if you're just using it to call a function, i.e. invoke(NULL, "runif", n = 10)

To me map(), map2(), and map_n() seem pretty well aligned. But I agree that it's confusing that invoke() and map_n() do basically the same thing (modulo argument order) on a transposed list.

(And both examples illustrate we need typed variants so you can get a character vector)

@lionel-

This comment has been minimized.

Copy link
Member Author

commented Nov 13, 2015

it looks a bit ugly if you're just using it to call a function, i.e. invoke(NULL, "runif", n = 10)

but that use case would be a rare occurrence right? hmm maybe not as rare for invoke_map().
But do.call() is a pretty common case so I think the syntax should favour it.

An alternative is to check for missingness of .x in addition to NULLness, so invoke(.f = "runif", n = 10) would work and it's still possible to do invoke(NULL, .f = "runif", 10).

@hadley

This comment has been minimized.

Copy link
Member

commented Nov 13, 2015

I think the problem is that map_invoke() is like map2() in the sense that neither of the arguments is primary. If you have a list of functions, you want the functions to be the first argument; if you have a list of parameters, you want the parameters to be the second argument. Either one form is going to look clumsier than the other, or we'll have to have two names for the different argument orders.

(I don't think we can automatically guess which input is which because currently we're allowing a character vector of function names as a short cut for a list of functions)

@lionel-

This comment has been minimized.

Copy link
Member Author

commented Nov 13, 2015

Either one form is going to look clumsier than the other

Yes. I think favouring do.call() use case and consistency with other mapping functions would be best. Also I think list of functions are less amenable to piping so don't need to be first argument as much.

But I trust your judgement. I guess you've been invoking list of functions more than I.

@jennybc

This comment has been minimized.

Copy link
Member

commented Nov 13, 2015

The more I think about it, the more I feel map_n() has a different structure than map() and map2().

Just about the name issue? I think this is why it never occurred to me that I could use map_n() to march through a data frame row by row until @lionel- told me outright. In my mind, map_n() is pmap(). I think about max()/min() vs. pmax()/pmin() in base R.

@hadley

This comment has been minimized.

Copy link
Member

commented Nov 13, 2015

Hmmmm, I like the pmax analogy although it's not quite right because you do pmax(x, y), not pmax(list(x, y)). map_n() is a bit like (the non-existent) colMax(). Regardless, I think pmap might be a better name than map_n since it invokes "parallel" which is so important here.

@hadley

This comment has been minimized.

Copy link
Member

commented Nov 14, 2015

@lionel- As I think about it more, I'm getting more comfortable with invoke() being a bit different. The primary use case is when you either have a list of functions you want to call, or you want to call the same function with different sets of arguments (i.e. you can't use map_n()). This means it's going to be used relatively rarely, and I think it's better to keep it distinct, rather than try and blurrr 😉 the boundary with the map functions.

@jennybc how would you feel about mapp() (for map, in parallel)? That would be more symmetric with map() and map2()

@jennybc

This comment has been minimized.

Copy link
Member

commented Nov 14, 2015

mapp() works for me.

@lionel-

This comment has been minimized.

Copy link
Member Author

commented Nov 15, 2015

the parallel idea is really good, though I think pmap() looks better. Also it reads differently than map() whereas "mapp" and "map" are pronounced the same.

@hadley

This comment has been minimized.

Copy link
Member

commented Nov 15, 2015

But then we have map(), map2() and pmap() which seems a bit inconsistent. OTOH pmap() does have a somewhat different interface to map() and map2() so maybe it's ok.

@hadley hadley closed this in d1eef0d Nov 18, 2015

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