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

notes on tune tokens #281

Closed
wants to merge 2 commits into from
Closed

notes on tune tokens #281

wants to merge 2 commits into from

Conversation

jakob-r
Copy link
Member

@jakob-r jakob-r commented Nov 13, 2020

talked with @be-marc. What do you think @mb706? Notes are in the source.

R/ObjectiveTuning.R Outdated Show resolved Hide resolved
# We might want to have the following in a function bc its used in MultiCrit as well
if (is.null(search_space)) {
# TODO: check if we can construct search space from learner$param_set using tune_tokens
tmp = learner$param_set$get_tune_pair() # return fixed_values (all but tune tokens) and search_space (from tune tokens),
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think we should have a function that returns two things that easily could be obtained by two calls, since here the two things that would happen are independent of each other.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just to emphasize that you always want to do both? If you want to have the search_space you also always should take the param_set with the remaining fixed param values.

@mb706
Copy link
Collaborator

mb706 commented Nov 13, 2020

  1. 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...).

  2. 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")
  3. 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.

@mb706
Copy link
Collaborator

mb706 commented Nov 13, 2020

so yeah I guess this solution is fine?

@mb706
Copy link
Collaborator

mb706 commented Nov 13, 2020

I implemented something that doesn't have to do the same thing in every .eval_many() call in #282 with tests

@jakob-r
Copy link
Member Author

jakob-r commented Nov 13, 2020

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?

@mb706
Copy link
Collaborator

mb706 commented Nov 13, 2020

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.

@mb706
Copy link
Collaborator

mb706 commented Nov 13, 2020

mlr-org/paradox#320 like this

@mb706
Copy link
Collaborator

mb706 commented Nov 13, 2020

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.

@mb706
Copy link
Collaborator

mb706 commented Nov 16, 2020

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 remove TuneToken for Learners (with tests) #282 because it has tests.

  • If no, then get_values drops TuneToken objects paradox#320 is probably the solution -- we would have to remove to_tune() from what the Learner code sees.

@be-marc
Copy link
Member

be-marc commented Jan 5, 2021

Fixed.

@be-marc be-marc closed this Jan 5, 2021
@be-marc be-marc deleted the tune_tokens branch January 24, 2022 08:16
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants