mlr-org / ParamHelpers

Helpers for parameters in black-box optimization, tuning and machine learning.
https://paramhelpers.mlr-org.com
Other
26 stars 9 forks source link

Remove warning when `NA` is set as default for a param? #217

Closed pat-s closed 4 years ago

pat-s commented 4 years ago

In

https://github.com/mlr-org/ParamHelpers/blob/391057d141cc1b104ca23934c92f49c60ce3a858/R/aParam.R#L127-L129

This comes up for learner xgboost in mlr and is quite annoying in the tests.

Has this fact ever triggered a serious problem? I don't think that the warning printed to the user is very helpful. It is tailored to devs and might confuse the user (the user can't do anything about it anyways). Especially because it comes up for learner xgboost (learner param "missing" has NA as default) it is somewhat important to get rid of it quickly.

makeLearner('classif.xgboost')
Warning in makeParam(id = id, type = "numeric", learner.param = TRUE, lower = lower,  :
  NA used as a default value for learner parameter missing.
ParamHelpers uses NA as a special value for dependent parameters.
Learner classif.xgboost from package xgboost
Type: classif
Name: eXtreme Gradient Boosting; Short name: xgboost
Class: classif.xgboost
Properties: twoclass,multiclass,numerics,prob,weights,missings,featimp
Predict-Type: response
Hyperparameters: nrounds=1,verbose=0

@jakob-r @berndbischl

berndbischl commented 4 years ago

puh. theres 2 things to say 1) there is conceptually i think no guarantee that PH works, when NA is used as a param value. the design defines NA as "missing" if a subordinate param is non-active. this is really bad, as demonstrated by your usecase above, when the value is actually used as a valid setting for a param. we have kinda inherited this problem now into paradox, where we made the same decision. i tried to avoid this, but couldn't find how. the problem are datatable representations. i have to put something into the cell-field of the atomic param column and NA seemed the only option.

2) NB: i am unaware of critical bugs that has every caused in practice but it still seems very bad design.

3) the warning is really of dubios value. i agree. i think we can remove it.

4) much more importantly: can we do something in paradox about this?

@mllg

pat-s commented 4 years ago

much more importantly: can we do something in paradox about this?

I initially suspected that you may have found a solution for this in paradox since it is the same in mlr3 for the xgboost learner just without a warning. But if not, it is even more worth discussing.

Maybe we can also agree that it is too complex too find an elegant solution (just hypothetical) seen that we've never seen an issue yet and we can't think of one directly.

berndbischl commented 4 years ago

we just dont warn in paradox, i think. but there is an issue from the initial creation of the package

https://github.com/mlr-org/paradox/issues/53

berndbischl commented 4 years ago

Maybe we can also agree that it is too complex too find an elegant solution (just hypothetical) seen that we've never seen an issue yet and we can't think of one directly.

i am pretty sure i could create a case that results in a bug

berndbischl commented 4 years ago

its just "obvious" that we are doing something not-so-great here. i know this. i just dont know an elegenat way to avoid this. @mllg said we should use attribs, to modify the meaning of the atomic values. thats great. that would solve this, i guess, neatly. then we remembered that attribs might be lost when indexing vectors. if thats true / still true, its absolute bullshit in R, but unfortunately also will not allow us to mahe use of it

berndbischl commented 4 years ago

the other option, the obvious one, is to store a "mask" somewhere that defines whether values are active or not. that either results in the datatable having more cols than params, i dislike this. or having 2 datatables.

berndbischl commented 4 years ago

actually........... i just looked at paradox.

a) we do not really use the NA semantics a lot there b) we mainly seem to wrap datatable in the Design-class. if we have a class anyway, i could add a second "mask" table.

berndbischl commented 4 years ago

or we REALLY forbid the use to use NAs in tuning. he can always map to an NA trough trafos. that might be easiest

berndbischl commented 4 years ago

IIRC the last option was suggested by @jakob-r and might be the best solution. if so, should be tested and documented and we then can also close the issue in paradox