mlr-org / mlrMBO

Toolbox for Bayesian Optimization and Model-Based Optimization in R
https://mlrmbo.mlr-org.com
Other
187 stars 47 forks source link

setMBOControl* should only overwrite with defaults if not already set. #76

Closed jakob-r closed 10 years ago

jakob-r commented 10 years ago

Maybe it's a bit annoying to implement it but i think it would be handy if the MBOControl setters just overwrote what's given and not the defaults. For example this should not happen.

> control = makeMBOControl()
> control = setMBOControlInfill(control = control, crit = "se")
> control = setMBOControlInfill(control = control, opt = "cmaes")
> control$infill.crit
[1] "mean"

Of course we can set it manually but then all the assertions and checks are in vain. What do you think?

jakobbossek commented 10 years ago

Hmm, not sure if this is worth the effort.

berndbischl commented 10 years ago

I like the OP suggestion better. It should not be hard to do?

BBmisc has insert and coalesce for this.

berndbischl commented 10 years ago

Just remove the default from the signature, and write

thearg = coalesce(thearg, 123) where 123 is the default?

jakobbossek commented 10 years ago

Ok.

jakobbossek commented 10 years ago

We have to remove all default values from the signature for the setMBOControl... and replace the default with NULL. If we do so, we have to introduce a lot of is.null checks before our sanity checks and assertions. With coalesce we than have to use (for example for the infill.crit parameter)

...
if (!is.null(infill.crit)) {
  assertChoice(crit, choices = getSupportedInfillCritFunctions())
}
...
control$infill.crit = coalesce(infill.crit, control$infill.crit, "mean")
berndbischl commented 10 years ago

Why dont you do

control$infill.crit = coalesce(infill.crit, control$infill.crit) # for this you set ALL defaults in makeMBOControl. assertChoice(control$infill.crit, choices = getSupportedInfillCritFunctions())

Less code and no "if" block.

jakobbossek commented 10 years ago

Well, this is of course far more elegant -.- Why should we set ALL defaults in makeMBOControl? This might be the better solution:

control$infill.crit = coalesce(infill.crit, control$infill.crit, "mean")
assertChoice(control$infill.crit, choices = getSupportedInfillCritFunctions())
berndbischl commented 10 years ago

Why should we set ALL defaults in makeMBOControl

You need to create a valid object after this call.

jakobbossek commented 10 years ago

Yes, I understand, but at the end of makeMBOControl we already call all the setMBOControl... funs. Because of this setting the default as above (as the last parameter to coalesce) should be ok.

berndbischl commented 10 years ago

Ok, the other option would be to remove these calls at the end of makeMBOControl. Do what you think is shorter and better

jakobbossek commented 10 years ago

Done.