tidymodels / dials

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

`default` values should be within `range` or `values` #229

Closed hfrick closed 2 years ago

hfrick commented 2 years ago

via @DavisVaughan

library(dials)
#> Loading required package: scales

# Issue: Shouldn't be able to set a default outside the range?
# Default should also always be a valid `value`? Or unknown()?
new_quant_param(
  type = "integer",
  default = 12L,
  values = c(1L, 5L, 10L),
  range = c(1L, 10L),
  inclusive = c(TRUE, TRUE),
  label = c(foo = "Foo")
)
#> Foo (quantitative)
#> Range: [1, 10]
#> Values: 3

Created on 2022-04-21 by the reprex package (v2.0.1)

DavisVaughan commented 2 years ago

Duplicate of #153 it seems

DavisVaughan commented 2 years ago
library(dials)
#> Loading required package: scales

# Laplace has a range argument but has a hardcoded default
x <- Laplace()
x$range
#> $lower
#> [1] 0
#> 
#> $upper
#> [1] 3
x$default
#> [1] 0

# So if you change the range...
x2 <- range_set(x, c(1, 3))

# You can end up with a default that is out of sync with the range
x2$range
#> $lower
#> [1] 1
#> 
#> $upper
#> [1] 3
x2$default
#> [1] 0

# We also have `range_validate()` which doesn't currently check the `default`
# either

# One option is to add a `default` arg to `range_set()` and all dials parameter
# helper functions and always require that they stay in sync
# range_set(x, c(1, 3), default = 2)
# Laplace(range = c(1, 3), default = 2)

# However, this is a lot of work for one thing, and is honestly a little more
# frustrating to use (because you have to think about the default so much).

# Since `default` is only used in `value_seq()` when `n == 1`, maybe we can
# just remove it?
# - When is `n == 1` used in practice?

Created on 2022-04-28 by the reprex package (v2.0.1)

DavisVaughan commented 2 years ago

used here https://github.com/tidymodels/dials/blob/6e63018d797e4b995919cc83627879be0305ebd0/R/aaa_values.R#L143

hfrick commented 2 years ago

note to self: we decided to deprecate the default argument

github-actions[bot] commented 2 years 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.