Remove warning when `NA` is set as default for a param?
Closed this issue · 9 comments
In
Lines 127 to 129 in 391057d
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=0puh. theres 2 things to say
-
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.
-
NB: i am unaware of critical bugs that has every caused in practice but it still seems very bad design.
-
the warning is really of dubios value. i agree. i think we can remove it.
-
much more importantly: can we do something in paradox about this?
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.
we just dont warn in paradox, i think. but there is an issue from the initial creation of the package
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
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
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.
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.
or we REALLY forbid the use to use NAs in tuning. he can always map to an NA trough trafos. that might be easiest
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