markfairbanks / tidytable

Tidy interface to 'data.table'
https://markfairbanks.github.io/tidytable/
Other
449 stars 33 forks source link

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

Closed RishavDaredevil closed 10 months ago

RishavDaredevil commented 10 months ago
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

markfairbanks commented 10 months ago

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 commented 10 months ago

thanks, I didn't know that.

RishavDaredevil commented 10 months ago

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

markfairbanks commented 10 months ago

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 commented 10 months ago

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