tidymodels / dials

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

Test is flaky #308

Closed MichaelChirico closed 11 months ago

MichaelChirico commented 11 months ago

This test can fail randomly:

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

library(dials)
library(testthat)

value_seq <- new_quant_param(
  type = "double",
  range = c(0.0, 1.0),
  inclusive = c(TRUE, TRUE),
  trans = NULL,
  values = (0:5) / 5,
  label = c(param = "param")
)
test_fails <- function(n = 40L) {
  value_sample(value_seq, n) |>
  unique() |>
  sort() |>
  expect_equal(value_seq$values) |>
  tryCatch(error = identity) |>
  inherits("error")
}
table(replicate(1000, test_fails()))
# FALSE  TRUE 
#   996     4 
MichaelChirico commented 11 months ago

If I up the sample size to 50L it is less than 0.1% failure:

table(replicate(5000, test_fails(50)))
# FALSE  TRUE 
#  4999     1
hfrick commented 11 months ago

Thanks for the issue and PR @MichaelChirico ! You may have been looking at an outdated version of this test though, that's not what it looks like on main now. I've changed it a while back in #297. I appreciate the contribution but I'm okay with the current version and am going to close the issue and PR.

MichaelChirico commented 11 months ago

not sure how I missed the updated test! I agree your implementation is much preferable since it has probability 1 of succeeding. thanks!

github-actions[bot] commented 11 months ago

This issue 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.