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

using pick() inside count() leads to an unhelpful error #778

Closed
RishavDaredevil opened this issue Nov 1, 2023 · 5 comments · Fixed by #779
Closed

using pick() inside count() leads to an unhelpful error #778

RishavDaredevil opened this issue Nov 1, 2023 · 5 comments · Fixed by #779

Comments

@RishavDaredevil
Copy link

data("mtcars")
    library(tidytable)
#> 
#> Attaching package: 'tidytable'
#> The following objects are masked from 'package:stats':
#> 
#>     dt, filter, lag
#> The following object is masked from 'package:base':
#> 
#>     %in%
    mtcars |>
        as_tidytable() |> 
        count(pick(contains("mpg")))
#> Error in `tidyselect_locs()`:
#> ! Problem while evaluating `pick(contains("mpg"))`.
#> Caused by error in `pick()`:
#> ! `pick()` can only work inside of tidytable verbs
#> Backtrace:
#>      ▆
#>   1. ├─tidytable::count(as_tidytable(mtcars), pick(contains("mpg")))
#>   2. │ └─tidytable:::count_tally(...)
#>   3. │   └─tidytable::summarize(...)
#>   4. │     └─tidytable:::tt_summarize(...)
#>   5. │       └─tidytable:::tidyselect_names(.df, !!.by)
#>   6. │         └─tidytable:::tidyselect_locs(.df, ...)
#>   7. │           └─tidyselect::eval_select(expr(c(...)), .df)
#>   8. │             └─tidyselect:::eval_select_impl(...)
#>   9. │               ├─tidyselect:::with_subscript_errors(...)
#>  10. │               │ └─rlang::try_fetch(...)
#>  11. │               │   └─base::withCallingHandlers(...)
#>  12. │               └─tidyselect:::vars_select_eval(...)
#>  13. │                 └─tidyselect:::walk_data_tree(expr, data_mask, context_mask)
#>  14. │                   └─tidyselect:::eval_c(expr, data_mask, context_mask)
#>  15. │                     └─tidyselect:::reduce_sels(node, data_mask, context_mask, init = init)
#>  16. │                       └─tidyselect:::walk_data_tree(new, data_mask, context_mask)
#>  17. │                         └─tidyselect:::eval_c(expr, data_mask, context_mask)
#>  18. │                           └─tidyselect:::reduce_sels(node, data_mask, context_mask, init = init)
#>  19. │                             └─tidyselect:::walk_data_tree(new, data_mask, context_mask)
#>  20. │                               └─tidyselect:::eval_context(expr, context_mask, call = error_call)
#>  21. │                                 ├─tidyselect:::with_chained_errors(...)
#>  22. │                                 │ └─rlang::try_fetch(...)
#>  23. │                                 │   ├─base::tryCatch(...)
#>  24. │                                 │   │ └─base (local) tryCatchList(expr, classes, parentenv, handlers)
#>  25. │                                 │   │   └─base (local) tryCatchOne(expr, names, parentenv, handlers[[1L]])
#>  26. │                                 │   │     └─base (local) doTryCatch(return(expr), name, parentenv, handler)
#>  27. │                                 │   └─base::withCallingHandlers(...)
#>  28. │                                 └─rlang::eval_tidy(as_quosure(expr, env), context_mask)
#>  29. └─tidytable::pick(contains("mpg"))
#>  30.   └─rlang::abort("`pick()` can only work inside of tidytable verbs")

Created on 2023-11-01 with reprex v2.0.2

@RishavDaredevil RishavDaredevil changed the title using pick inside count leads to a error using pick inside count leads to a error Nov 1, 2023
@markfairbanks
Copy link
Owner

markfairbanks commented Nov 1, 2023

This is actually a difference between tidytable and dplyr. You don't need pick() in functions like count() or group_by(). You can directly use tidyselect functions like contains(), where(), etc.

library(tidytable)

mtcars |>
  as_tidytable() |> 
  count(contains("mpg")) |>
  head()
#> # A tidytable: 6 × 2
#>     mpg     n
#>   <dbl> <int>
#> 1  10.4     2
#> 2  13.3     1
#> 3  14.3     1
#> 4  14.7     1
#> 5  15       1
#> 6  15.2     2


mtcars |>
  as_tidytable() |>
  group_by(contains("mpg")) |>
  count() |>
  head()
#> # A tidytable: 6 × 2
#> # Groups:      mpg
#>     mpg     n
#>   <dbl> <int>
#> 1  10.4     2
#> 2  13.3     1
#> 3  14.3     1
#> 4  14.7     1
#> 5  15       1
#> 6  15.2     2

I'll add an error message to count() - I currently do that in group_by() if you try to use pick().

library(tidytable)

mtcars |>
  as_tidytable() |>
  group_by(pick(contains("mpg")))
#> Error in `check_across()`:
#> ! `across()`/`pick()` are unnecessary in `group_by()`.
#> Please directly use tidyselect.
#> Ex: df %>% group_by(where(is.numeric))
#> Backtrace:
#>     ▆
#>  1. ├─tidytable::group_by(as_tidytable(mtcars), pick(contains("mpg")))
#>  2. └─tidytable:::group_by.tidytable(as_tidytable(mtcars), pick(contains("mpg")))
#>  3.   └─tidytable:::check_across(dots, "group_by")
#>  4.     └─rlang::abort(msg)

@RishavDaredevil
Copy link
Author

thanks, I didn't know that.

@RishavDaredevil
Copy link
Author

should I close the issue @markfairbanks this is my first time asking a question on GitHub so I don't know the dynamics?

@markfairbanks
Copy link
Owner

markfairbanks commented Nov 1, 2023

Can you keep it open? I'm going to use it so I update the error message for this case.

Typically I'll close an issue if I consider it "resolved" (which most maintainers will do the same). Sometimes if you're the user and you think it's resolved you can close it if they don't.

This is just sort of an odd case where the behavior is expected, but a better error message would still be helpful.

@markfairbanks markfairbanks changed the title using pick inside count leads to a error using pick() inside count() leads to an unhelpful error Nov 1, 2023
@markfairbanks
Copy link
Owner

Just updated the error for the next release - thanks for catching this!

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

Successfully merging a pull request may close this issue.

2 participants