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

set_params #74

Closed
11 tasks
sebffischer opened this issue Jul 26, 2022 · 3 comments
Closed
11 tasks

set_params #74

sebffischer opened this issue Jul 26, 2022 · 3 comments

Comments

@sebffischer
Copy link
Sponsor Member

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

  • Learner
  • Graph
  • PipeOp
  • Measure
  • Resampling
  • TaskGenerator
  • Optimizer
  • Terminator
  • Tuner
  • Surrogate
  • AcqOptimizer

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

@mllg
Copy link
Sponsor Member

mllg commented Jul 27, 2022

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
Copy link
Sponsor Member Author

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
Copy link
Sponsor Member Author

Ok actually I fully agree with you.

@mllg mllg closed this as completed Sep 21, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants