mlr-org / mlr3misc

Miscellaneous helper functions for mlr3
https://mlr3misc.mlr-org.com
GNU Lesser General Public License v3.0
12 stars 3 forks source link

set_params #74

Closed sebffischer closed 2 years ago

sebffischer commented 2 years ago

I think we should really offer this set_params public method for Learners, Graphs etc. so we can use it in the book. Last time @mllg mentioned that we could just offer a s3 method that does something akin to

set_params.R6 = function(x, ...) {
  if (!is.null(x$param_set)) {
    x$param_set$values = insert_named(x$param_set$values, list(...))
  } 
  invisible(x)
}

admittedly that is less work and we could simply add it in mlr3misc without touching other packages. But I think the downsight is that this is isomewhat inconsistent. The user then has to guess when something is implemented as a method vs a s function (?) I think offering both would be fine as well but I definitely think that the following should work.

learner$set_params(x = 1, y = 2) 

This is also related to the suggestion that @mb706 made at some point, that it might make sense to introduce another class into the hierarchy that represents an object that has a param set and some other standardized properties? But even if this would make sense it would definitely take some time so I would suggest we make an mlr3misc release with this set_params utility that is already implemented and then add the method set_params to

I am not sure whether I forgot something. I can do this if you are ok @mllg @be-marc

mllg commented 2 years ago

If you want to have a method, how about implementing the method in the param set, e.g. learner$param_set$set_params()? It wouldn't make sense to have all methods to work with parameters inside param_set except one.

sebffischer commented 2 years ago

Good point, on the other hand this is really the only way a user ever interacts with the param set so maybe it is ok to make this exception / do both?

I feel that learner$set_params(x = 1) is more user friendly than learner$param_set$set_values(x = 1) but I would be ok with your suggestion.

In any case as a first step I would add the method and call it set_values to make it consistent with the getter get_values.

sebffischer commented 2 years ago

Ok actually I fully agree with you.