search

tidymodels / dials

Tools for creating tuning parameter values
https://dials.tidymodels.org/
Other
111 stars 26 forks source link

Use expect_in() for weaker but surer test #310

Closed MichaelChirico closed 11 months ago

MichaelChirico commented 11 months ago

Follow-up to #309.

I also saw these tests using sort(unique()), but I don't have enough context to know whether it's preferable to switch those to expect_in() as well:

https://github.com/tidymodels/dials/blob/55763e0cbd49a16a3f5a532dc92b9069258d54e7/tests/testthat/test-aaa_values.R#L264-L271

MichaelChirico commented 11 months ago

A very ad hoc benchmark suggests we should skip the unique() step, especially for readability:

x = sample(letters, 100, TRUE)
microbenchmark(times = 10000, all(unique(x) %in% letters), all(x %in% letters))
# Unit: microseconds
#                         expr   min      lq     mean median      uq       max neval cld
#  all(unique(x) %in% letters) 8.209 16.7145 25.84045 17.485 18.6015 47076.585 10000   b
#          all(x %in% letters) 4.289  9.1590 12.61616  9.734 10.5710  3739.765 10000  a
hfrick commented 11 months ago

Thanks!

github-actions[bot] commented 11 months ago

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