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

Add mapper support for compose #556

Closed
ColinFay opened this issue Oct 2, 2018 · 11 comments
Closed

Add mapper support for compose #556

ColinFay opened this issue Oct 2, 2018 · 11 comments

Comments

@ColinFay
Copy link
Contributor

ColinFay commented Oct 2, 2018

Issue

Today, if I want to insert mappers in a compose call, I'll have to wrap it into :

sample_round <- compose(
  as_mapper(~ round(.x, 2)),
  mean,
  as_mapper(~ sample(.x, 10))
)
map_dbl(mtcars, sample_round)
   mpg    cyl   disp     hp   drat     wt   qsec     vs     am   gear   carb 
 20.10   6.20 240.62 139.00   3.46   2.92  17.35   0.50   0.40   3.80   2.50 

As this is not supported:

sample_round <- compose(
  ~ round(.x, 2),
  mean,
  ~ sample(.x, 10)
)
Error in FUN(X[[i]], ...) : 
  'list(...)[[1L]]' is not a function, character or symbol

This is because compose uses lapply(list(...), match.fun), and match.fun doesn't know how to deal with formulas.

Proposed solution

I suggest a modification for :

compose <- function(...) {
  fs <- lapply(list(...), as_mapper)
  n <- length(fs)
  
  last <- fs[[n]]
  rest <- fs[-n]
  
  function(...) {
    out <- last(...)
    for (f in rev(rest)) {
      out <- f(out)
    }
    out
  }
}

sample_round <- compose(
  ~ round(.x, 2),
  mean,
  ~ sample(.x, 10)
)
map_dbl(mtcars, sample_round)
   mpg    cyl   disp     hp   drat     wt   qsec     vs     am   gear   carb 
 21.77   5.80 293.78 125.90   3.53   3.27  17.87   0.30   0.70   3.60   2.30 

This would also allow constructing complex "pluckers" :

plop <- list(
  a = list(
    b = list(
      c = list(
        10, 20, 30
        )
    )
  ), 
  aa = list(
    b = list(
      c = list(
        10, 20, 30
      )
    )
  ), 
  aaa = list(
    b = list(
      c = list(
        10, 20, 30
      )
    )
  )
)

plucker <- compose(~ .x * 10, 1, "c", "b")
map(plop, plucker)
$a
[1] 100

$aa
[1] 100

$aaa
[1] 100

Downside

Current version of compose allows to compose with characters elements. I'm not sure if this is widely used though (the documentation do not reference this use case, so I guess we can safely assume this is seldom used):

new_compose <- purrr::compose("round", "mean")
map_dbl(mtcars, new_compose)
 mpg  cyl disp   hp drat   wt qsec   vs   am gear carb 
  20    6  231  147    4    3   18    0    0    4    3 
@egnha
Copy link
Contributor

egnha commented Oct 2, 2018

@ColinFay, I'm not sure whether this well documented, but map() already applies as_mapper(), so your plucker can be expressed more succinctly like this:

map(plop, list("b", "c", 1)) %>% str
#> List of 3
#> $ a  : num 10
#> $ aa : num 10
#> $ aaa: num 10

As for your other compose() example, may I suggest using gestalt::compose() instead, or better yet, the composition operator gestalt::`%>>>%`? The operator %>>>% comprehends the magrittr %>% syntax, so instead of

sample_round <- purrr::compose(
  as_mapper(~ round(.x, 2)),
  mean,
  as_mapper(~ sample(.x, 10))
)

you can write

library(gestalt)

sample_round <- sample(10) %>>>% mean %>>>% round(2)

set.seed(1)
purrr::map_dbl(mtcars, sample_round)
#>   mpg    cyl   disp     hp   drat     wt   qsec     vs     am   gear   carb 
#> 19.68   6.40 169.12 153.60   3.48   3.28  17.69   0.50   0.20   3.90   2.20 

Like %>%, the composition is read from left to right.

@ColinFay
Copy link
Contributor Author

ColinFay commented Oct 2, 2018

@egnha ah, my example was indeed poorly chosen, I had a case lately where I wanted to do something which would require a mix of index extraction and function modification:

So maybe something more like:

# Modified version of compose
plucker <- compose(~ .x * 10, 1, "c", "b")
map(plop, plucker)
$a
[1] 100

$aa
[1] 100

$aaa
[1] 100

Which doesn't work with:

map(plop, list("b", "c", 1, ~ .x * 10))
Error: Index 4 must have length 1

I'll update my issue.


Thanks for pointing {gestalt}, I'll have a look!

@egnha
Copy link
Contributor

egnha commented Oct 2, 2018

The "mapper composition" you want is a composition of higher-order functions. So instead of modifying compose(), you can implement it declaratively:

library(purrr)
library(gestalt)

mcompose <- list %>>>% map(as_mapper) %>>>% gestalt::compose

Note that such an expression works with gestalt::compose(), but not with purrr::compose(), because the former accepts both ... and lists of functions, while the latter doesn't —you'd have to invoke do.call().

I believe this does what you want:

map(plop, mcompose("b", "c", 1, ~.x * 10)) %>% str
#> List of 3
#>  $ a  : num 100
#>  $ aa : num 100
#>  $ aaa: num 100

(Actually, in this particular example, it would be better to do

map_dbl(plop, list("b", "c", 1)) * 10

optionally followed by as.list(), because * is vectorized.)

@ColinFay
Copy link
Contributor Author

ColinFay commented Oct 2, 2018

@egnha yep, this totally works for these fringe use cases, thanks for the advice :)

Still, I think adding mapper support to compose would be consistent with the rest of the package

@lionel-
Copy link
Member

lionel- commented Oct 2, 2018

We'd have to deprecate string-matching first so it'd make more sense to use as_function() I think, at least at first.

However if you're using the lambda formula already, isn't it simpler and more readable to just construct the function you need directly?

@ColinFay
Copy link
Contributor Author

ColinFay commented Oct 3, 2018

I can indeed use a function to do that.
Yet it seems more coherent and homogeneous to write (sorry for the silly example):

library(purrr)
library(dplyr)

random_mean <- compose(round, mean, ~ sample(.x, 10))
rounded_sd  <- compose(round, sd)

iris %>%
  group_by(Species) %>%
  summarize(rm = random_mean(Sepal.Length), 
            rs = rounded_sd(Sepal.Length))

Than :

rounded_sd  <- compose(round, sd)
random_mean <- function(x){
  sample(x) %>% 
    mean() %>%
    round()
}
# Or even 
rounded_sd  <- compose(round, sd)
random_mean <- as_mapper(~ round(mean(sample(.x, 10))))

iris %>%
  group_by(Species) %>%
  summarize(rm = random_mean(Sepal.Length), 
            rs = rounded_sd(Sepal.Length))

Also, it's a situation I've faced several times in training when I showed {purrr}:

  • I show mappers and how they work, they learn how to use it
  • then we move to safely and possibly, which still handle mappers,
  • detect and has_element are still natively handling mappers
  • then we move to compose, and here mappers do not natively works, we have to wrap them with as_mapper inside compose, or switch back to a simple mapper with a lot of () / %>% in it, or switch back to writing a function.
  • then accumulate, and it natively supports mappers

@lionel-
Copy link
Member

lionel- commented Oct 3, 2018

I was talking about plucking support, i.e. using as_mapper() instead of as_function(). I think it's just simpler and more readable to do extraction within a custom lambda rather than composing plucker and functions with compose().

@lionel-
Copy link
Member

lionel- commented Oct 3, 2018

And ~ support can be implemented with as_function() without breaking the interface. Plucking support is a breaking change so should only be done if useful.

@ColinFay
Copy link
Contributor Author

ColinFay commented Oct 3, 2018

Ah, ok.

It indeed seems like a very rare use case (this kind of plucking where you need to combine lambda and numeric/characters — I had to do this twice and I indeed implemented my own function to do that).

Not worth the breaking change if as_function() could do the trick without breaking anything, my original suggestion was more about being able to use lamba inside compose for composing function :)

Should I make a PR with the modified version of compose with as_function?

@lionel-
Copy link
Member

lionel- commented Oct 3, 2018

That'd be great! It should still use match.fun() for strings.

@edavidaja
Copy link
Contributor

I posted this issue at furrr initially, but I'm wondering if it's related to this discussion--essentially, I'm trying to compose with pluck, and it works with map() but not future_map() or future_lapply(). Happy to start a separate issue if it's unrelated.

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

4 participants