tidymodels / dials

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

default values for tuning parameters should be within range #153

Closed lukasal closed 2 years ago

lukasal commented 3 years ago

The problem

The default value for parameters should be within range, as otherwise out of range values are selected (e.g. in value_seq_int and value_seq_double when levels == 1), but also everywhere else where the default is accessed.

Reproducible example

library(tidymodels)
param <- under_ratio(range = c(12,13))
param$default
# the value is 1, it should been something between 12,13
grid_regular(param,levels = 1)
# with the randomized grid it works
grid_random(param,size = 1)

reprex::reprex(si = TRUE)
juliasilge commented 3 years ago

It looks like you may have had trouble getting started with reprex; take a look at this article for a walk-through of how to use it.

library(dials)
#> Loading required package: scales

param <- under_ratio(range = c(12, 13))
param$default
#> [1] 1

grid_regular(param, levels = 1)
#> # A tibble: 1 x 1
#>   under_ratio
#>         <dbl>
#> 1           1

grid_random(param, size = 1)
#> # A tibble: 1 x 1
#>   under_ratio
#>         <dbl>
#> 1        12.7

Created on 2021-02-17 by the reprex package (v1.0.0)

This is happening because in value_seq_dbl(), this first condition is evaluating to TRUE and the object default is being used:

https://github.com/tidymodels/dials/blob/6e939ff4ca3906972c131c92c6dfa52226f932ce/R/aaa_values.R#L142

What we want to happen is to get down to this else instead:

https://github.com/tidymodels/dials/blob/6e939ff4ca3906972c131c92c6dfa52226f932ce/R/aaa_values.R#L147-L153

lukasal commented 3 years ago

Thanks for the reply! The fix depends on whether it is intended to use the default value in that case or the range. There might be other dependencies on the default value for other use cases and in other functions (other value_seq functions), but I don't know the code well enough to estimate this. So one either changes the default or fix the dependency on the default.

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.