-
-
Notifications
You must be signed in to change notification settings - Fork 50
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
Add randomPlantedForest learners (classif, regr) #304
Conversation
Install this branch for a hacky roxygen workaround: https://github.com/mlr-org/mlr3misc/tree/fix/document-leanify |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks a lot! I have mostly some minor comments
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
very minor things...
tests/testthat/test_paramtest_randomPlantedForest_classif_rpf.R
Outdated
Show resolved
Hide resolved
Thanks! Okay, I think the only thing left is to figure out what to do about the custom parameters (#304 (comment)). |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Some minor issues still left, lmk when you are confused about something.
#' The parameter `max_interaction_limit` can optionally be set as an upper bound, such that | ||
#' `max_interaction_ratio * min(n_features, max_interaction_limit)` is used instead. | ||
#' This is analogous to `mtry.ratio` in [`classif.ranger`][mlr3learners::mlr_learners_classif.ranger], with | ||
#' `max_interaction_limit` as an additional constraint. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
mention that max_interaction_limit
is initialized to Inf
Co-authored-by: Sebastian Fischer <sebf.fischer@gmail.com>
Co-authored-by: Sebastian Fischer <sebf.fischer@gmail.com>
Co-authored-by: Sebastian Fischer <sebf.fischer@gmail.com>
Co-authored-by: Sebastian Fischer <sebf.fischer@gmail.com>
Co-authored-by: Sebastian Fischer <sebf.fischer@gmail.com>
param_set = ps( | ||
max_interaction = p_int(lower = 0, upper = Inf, default = 1, tags = "train"), | ||
max_interaction_ratio = p_dbl(lower = 0, upper = 1, tags = "train"), | ||
max_interaction_limit = p_int(lower = 1, upper = Inf, tags = c("required", "train")), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
max_interaction_limit = p_int(lower = 1, upper = Inf, tags = c("required", "train")), | |
max_interaction_limit = p_int(lower = 1, upper = Inf, tags = c("required", "train"), special_vals = list(Inf)), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
sorry I broke something
Co-authored-by: Sebastian Fischer <sebf.fischer@gmail.com>
This adds the
randomPlantedForest
learners for classification and regression.I started this fork some time ago to use the learners in a benchmark and it's high time to make it official I guess.
There are still some things to do though:
DESCRIPTION