tidymodels / dials

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

print an indicator when "values" is set for quant params #138

Closed kmdupr33 closed 4 years ago

kmdupr33 commented 4 years ago

Fixes #24

topepo commented 4 years ago

Maybe @DavisVaughan or @juliasilge feel differently but I'd like to remove the range of data in values. That wouldn't get used anywhere (programmatically) and might cause confusion. I also like the output of these functions to be succinct.

juliasilge commented 4 years ago

So @topepo am I understanding correctly that you are suggesting to have the output look like this only, without indication of the set values:

#> Cost-Complexity Parameter  (quantitative)
#> Transformer:  log-10 
#> Range (transformed scale): [-10, -1]
#> Values: 2

I am guessing that @DavisVaughan at least mildly disagrees since printing the set range was one of the points of the original issue. For me, I'm not sure why the number of values is more helpful than the range, if succinctness is the concern. I'd probably prefer to see the new range added over the number of values if there is a strong reason to restrict adding new roles in the print output.

🎯 What do you all think about "Values Range" vs. something like "Current Range:" or "Set Range:"?

DavisVaughan commented 4 years ago

I am fine with just printing Values: N in addition to the original Range.

@juliasilge I now think that saying there is a "new range" is a little misleading. The range is the same as it was before, there are now just a limited number of values that can be pulled from that range (which is all I wanted to be notified of). Note the difference between range_set() and value_set()

library(dials)
#> Loading required package: scales

cp <- cost_complexity()

cp
#> Cost-Complexity Parameter  (quantitative)
#> Transformer:  log-10 
#> Range (transformed scale): [-10, -1]

# Range does update here
range_set(cp, c(-5, -2))
#> Cost-Complexity Parameter  (quantitative)
#> Transformer:  log-10 
#> Range (transformed scale): [-5, -2]

# This specifies that there are 4 allowed values
# and all of them are within the printed Range
value_set(cp, c(-5, -4, -2, -1.5))
#> Cost-Complexity Parameter  (quantitative)
#> Transformer:  log-10 
#> Range (transformed scale): [-10, -1]

# The Range is still useful, as it enforces
# the that the 4 allowed values are still within that range
value_set(cp, c(-5, -4, -2, 5))
#> Error: Some values are not valid: 5
topepo commented 4 years ago

Here's my rationale... a lot of our code uses the parameter range to define grids, create random values etc. This could be different from the range of the values.

It might lead users seeing the value range to get the wrong idea about what the other functions would use.

juliasilge commented 4 years ago

Ah, got it! That clears that up for me. So we want the output to be:

#> Cost-Complexity Parameter  (quantitative)
#> Transformer:  log-10 
#> Range (transformed scale): [-10, -1]
#> Values: 2
DavisVaughan commented 4 years ago

Yes, @kmdupr33 does that make sense? Could you please make those changes for us? (Mainly removing the values range output part)

kmdupr33 commented 4 years ago

Yes, @kmdupr33 does that make sense? Could you please make those changes for us? (Mainly removing the values range output part)

Yep. That's an easy change. I'll have that done by today for sure. :)

kmdupr33 commented 4 years ago

@DavisVaughan, I've made the change.

DavisVaughan commented 4 years ago

Thanks @kmdupr33!

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