Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Conversation

kmdupr33
Copy link
Contributor

@kmdupr33 kmdupr33 commented Aug 8, 2020

Fixes #24

R/constructors.R Outdated Show resolved Hide resolved
@topepo
Copy link
Member

topepo commented Aug 12, 2020

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
Copy link
Member

juliasilge commented Aug 12, 2020

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
Copy link
Member

DavisVaughan commented Aug 13, 2020

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
Copy link
Member

topepo commented Aug 13, 2020

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
Copy link
Member

juliasilge commented Aug 13, 2020

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
Copy link
Member

DavisVaughan commented Aug 13, 2020

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

@kmdupr33
Copy link
Contributor Author

kmdupr33 commented Aug 13, 2020

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
Copy link
Contributor Author

kmdupr33 commented Aug 13, 2020

@DavisVaughan, I've made the change.

@DavisVaughan DavisVaughan force-pushed the quant_param_print_indicator_when_values_set_24 branch from dfe4ad1 to 111ad63 Compare Aug 17, 2020
@DavisVaughan DavisVaughan merged commit 71ce06f into tidymodels:master Aug 17, 2020
@DavisVaughan
Copy link
Member

DavisVaughan commented Aug 17, 2020

Thanks @kmdupr33!

@github-actions
Copy link

github-actions bot commented Mar 6, 2021

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.

@github-actions github-actions bot locked and limited conversation to collaborators Mar 6, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Should quant parameters print an indicator when values is set?
4 participants