Closed jakob-r closed 3 years ago
There is a general question, independent of to_tune, that we should consider first: What do we do if the learner that is given has two hyperparameters, lets say X
and Y
both "logical". Y
depends on X
being TRUE
. The user specifies (without the to_tune mechanism) the correct search space, but the learner object that is given to the tuner has X
set to FALSE
. Should tuning fail? Is it the responsibility of the user to make sure that values of the learner that is being tuned over are unset? There is a case for this, because in principle we cannot tell from a searchspace what parameters it is going to set (because its trafo could do anything, and we don't have codomain info for tuning paramset trafos (although the technology exists...).
Suppose we say the answer to 1. is "the $values
of parameters being tuned should be empty". In that case we obviously wouldn't want to put the burden on the user to remove the TuneToken
objects from the learner. In that case I would say the best solution is to remove all TuneToken
objects from the Objective$learner$param_set$values
ourselves, after this line:
self$learner = assert_learner(as_learner(learner, clone = TRUE),
task = self$task)
# add the following
self$learner$param_set$values = discard(self$learner$param_set$values, inherits, "TuneToken")
Suppose the answer to 1. is "If $values
are set before tuning, then things should still work" (Again, this is independent of to_tune
). Then we can consider fixing this inside paradox
. For example by setting a $values
slot to NULL
, instead of erroring, when its dependency is not fulfilled.
I would prefer doing 2., because it is the minimally invasive change, but I can see an argument for 3.
so yeah I guess this solution is fine?
I implemented something that doesn't have to do the same thing in every .eval_many()
call in https://github.com/mlr-org/mlr3tuning/pull/282 with tests
I don't like the above approach of having to delete TuneTokens in mlr3tuning because what if tune tokens are in the learner but the search_space is actually created with different params. We would have to catch that in a weird place. I think a clean API between Paradox and mlr3tuning would hide the existence of TuneTokens to mlr3tuning.
Also do we risk having to write this delete tokens line in other packages?
what if tune tokens are in the learner but the search_space is actually created with different params
it is still good to delete the tune_tokens. the learner never uses tune_tokens, so nothing is lost.
We would have to catch that in a weird place
We just ignore it
hide the existence of TuneTokens to mlr3tuning
Also do we risk having to write this delete tokens line in other packages?
That is a legitimate concern / question. We could address this by having ParamSet$get_values()
ignoring TuneToken
objects. The error that comes is not because of dependency checks inside paradox, but because the learner gets a TuneToken
object and doesn't know what to do with it. I think it would be good hygiene to always use $get_values()
inside learners, to distinguish it from the user changing values. This would enable param_set-side trafos as well.
So far we have mostly implicitly relied on "the user uses ParamSet$values
to set parameters, the Learner (or object being configured) should use $get_values()
to get the parameters". This would make it explicit, which I am in favour of.
Coming back to
Also do we risk having to write this delete tokens line in other packages?
The question comes down to: If the user sets something to to_tune()
, and then runs the learner without tuning, should that be an error?
If yes, then we do need special purpose code in mlr3tuning
, and in every other place that converts a Learner
from "having to_tune()
in the $values
is an error" to "having to_tune()
in the $values
is OK". If we go this route, then we should go with what Marc already implemeneted / merge #282 because it has tests.
If no, then https://github.com/mlr-org/paradox/pull/320 is probably the solution -- we would have to remove to_tune()
from what the Learner
code sees.
Fixed.
talked with @be-marc. What do you think @mb706? Notes are in the source.