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

summarise_all applies functions to themselves #3094

Closed
flying-sheep opened this issue Sep 11, 2017 · 15 comments
Closed

summarise_all applies functions to themselves #3094

flying-sheep opened this issue Sep 11, 2017 · 15 comments
Assignees

Comments

@flying-sheep
Copy link

@flying-sheep flying-sheep commented Sep 11, 2017

> dplyr::summarise_all(data.frame(wat = letters), function(col) sum(!is.na(col)))
  wat
1   1
Warning message:
In is.na(col) : is.na() applied to non-(list or vector) of type 'closure'
> dplyr::summarise_all(data.frame(wat = letters), dplyr::funs(function(col) sum(!is.na(col))))
Error in summarise_impl(.data, dots) : 
  Column `wat` is of unsupported type function
Calls: <Anonymous> ... summarise -> summarise.tbl_df -> summarise_impl -> .Call
@JohnMount
Copy link

@JohnMount JohnMount commented Sep 11, 2017

I also see errors. But, I see a different pattern of what works and does not work (following the 3 ways to specify a function mentioned in help(funs): by name, the function, or an expression over "."). Notice the if you store your function in a variable and then refer to the variable, that works (possible work-around, but according to the documentation you should be able to define the function inline as you did).

suppressPackageStartupMessages(library("dplyr"))

dplyr::summarise_all(data.frame(wat = letters), 
                     sum(!is.na(.)))
#> Error in inherits(x, "fun_list"): object '.' not found

f <- function(col) sum(!is.na(col))
dplyr::summarise_all(data.frame(wat = letters), 
                     f)
#>   wat
#> 1  26

f <- function(col) sum(!is.na(col))
dplyr::summarise_all(data.frame(wat = letters), 
                    function(col) sum(!is.na(col)))
#>   wat
#> 1  26


dplyr::summarise_all(data.frame(wat = letters), 
                     dplyr::funs(sum(!is.na(.))))
#>   wat
#> 1  26

f <- function(col) sum(!is.na(col))
dplyr::summarise_all(data.frame(wat = letters), 
                     dplyr::funs(f))
#>   wat
#> 1  26

f <- function(col) sum(!is.na(col))
dplyr::summarise_all(data.frame(wat = letters), 
                     dplyr::funs(function(col) sum(!is.na(col))))
#> Error in summarise_impl(.data, dots): Column `wat` is of unsupported type function

packageVersion("dplyr")
#> [1] '0.7.3'

Loading

@lionel-
Copy link
Member

@lionel- lionel- commented Nov 4, 2017

@flying-sheep Could you try again on master please? I can't reproduce the issue.

@JohnMount It looks like all these cases work as expected.

Loading

@flying-sheep
Copy link
Author

@flying-sheep flying-sheep commented Nov 13, 2017

With master I now get:

> dplyr::summarise_all(data.frame(wat = letters), function(col) sum(!is.na(col)))
  wat
1  26
> dplyr::summarise_all(data.frame(wat = letters), dplyr::funs(function(col) sum(!is.na(col))))
Fehler in summarise_impl(.data, dots) : 
  Column `wat` is of unsupported type function

so at least the first one was eliminated!

Loading

@lionel-
Copy link
Member

@lionel- lionel- commented Nov 13, 2017

I don't think the second one is supposed to work. I guess we could special-case function. What do you think @hadley?

Loading

@flying-sheep
Copy link
Author

@flying-sheep flying-sheep commented Nov 13, 2017

the docs say:

     ...: A list of functions specified by:Their name, ‘"mean"’
            • The function itself, ‘mean’
            • A call to the function with.as a dummy argument,
              ‘mean(., na.rm = TRUE)’

the second one says functions are accepted.

Loading

@lionel-
Copy link
Member

@lionel- lionel- commented Nov 13, 2017

yes but funs() is a quoting function so you're not supplying a function but just an expression. The second bullet should be "The function name".

Loading

@hadley
Copy link
Member

@hadley hadley commented Nov 13, 2017

Agreed. We should clarify the docs for this case. Maybe:

A list of functions specified by:

  • Their name as a string, "mean"
  • The bare function function name, e.g. mean
  • A function call using . as a pronoun to refer to the input data, e.g. mean(., na.rm = TRUE)

Loading

@krlmlr
Copy link
Member

@krlmlr krlmlr commented Mar 16, 2018

It feels inconsistent that funs(mean) works but funs(function(x) mean(x)) or even funs(mean = function(x) mean(x)) doesn't. For now let's update the docs and explicitly mention that passing an anonymous function won't work.

Loading

@lionel-
Copy link
Member

@lionel- lionel- commented Mar 16, 2018

If we could go back in time we'd disallow funs(mean) I think.

Loading

@krlmlr
Copy link
Member

@krlmlr krlmlr commented Mar 16, 2018

Reference: #3368 (comment).

Loading

@hadley
Copy link
Member

@hadley hadley commented Mar 16, 2018

If we could do back in time, I think we'd disallow funs(mean(., na.rm = TRUE)) and require that all argument be functions (or the formula shortcut)

Loading

@lionel-
Copy link
Member

@lionel- lionel- commented Mar 16, 2018

We wouldn't need funs() at all then.

Loading

@lionel-
Copy link
Member

@lionel- lionel- commented Mar 16, 2018

Hmm does this mean we should start deprecating funs() at some point?

Loading

@krlmlr
Copy link
Member

@krlmlr krlmlr commented Mar 16, 2018

Let's move discussion to the issue I just created.

Loading

@krlmlr krlmlr closed this in b040594 Mar 16, 2018
krlmlr added a commit that referenced this issue Mar 16, 2018
* Show clear error message for bad arguments to `funs()` (#3368).

* Improved documentation for `funs()` (#3094).

* Compute variable names for joins in R (#3430).

* Hybrid evaluation simplifies `dplyr::foo` to `foo` (#3309).

* `bind_cols()` handles unnamed list (#3402).
@lock
Copy link

@lock lock bot commented Sep 12, 2018

This old issue has been automatically locked. If you believe you have found a related problem, please file a new issue (with reprex) and link to this issue. https://reprex.tidyverse.org/

Loading

@lock lock bot locked and limited conversation to collaborators Sep 12, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

None yet
5 participants