-
-
Notifications
You must be signed in to change notification settings - Fork 402
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
Move pkg function calls within ParamSets to helper file #2730
Conversation
It looks a bit cumbersome. |
Thanks. Seems to work. |
Sorry. This was a stupid suggestion. Now we will run into problems if we want to set e.g. mlr/R/RLearner_classif_fdausc.np.R Lines 39 to 40 in 7ebc364
Otherwise just use |
This is within I will use the |
R/RLearner_classif_mda.R
Outdated
@@ -21,7 +21,7 @@ makeRLearner.classif.mda = function() { | |||
makeLogicalLearnerParam(id = "trace", default = FALSE, tunable = FALSE), | |||
makeDiscreteLearnerParam(id = "start.method", default = "kmeans", values = c("kmeans", "lvq")), | |||
makeIntegerLearnerParam(id = "tries", default = 5L, lower = 1L), | |||
makeDiscreteLearnerParam(id = "criterion", default = "misclassification", values = c("misclassification", "deviance")) | |||
makeDiscreteLearnerParam(id = "criterion", default = "misclassification", special.vals = c("misclassification", "deviance")) |
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.
makeDiscreteLearnerParam(id = "criterion", default = "misclassification", special.vals = c("misclassification", "deviance")) | |
makeDiscreteLearnerParam(id = "criterion", default = "misclassification", values = c("misclassification", "deviance")) |
special.vals
are meant for values that do not match the param type. It also does not make much sense to define a DiscreteParam without discrete values.
R/RLearner_classif_mda.R
Outdated
values = list(polyreg = mda::polyreg, mars = mda::mars, bruto = mda::bruto, gen.ridge = mda::gen.ridge)), | ||
# see helpers_mda.R for objects | ||
makeUntypedLearnerParam(id = "method", default = function(...) mda::polyreg(...), | ||
values = list(polyreg = function(...) mda::polyreg(...), |
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.
for an untyped Param you cannot set values
.
Now everyhting looks mixed up... |
Codecov Report
@@ Coverage Diff @@
## master #2730 +/- ##
=========================================
Coverage ? 92.36%
=========================================
Files ? 402
Lines ? 14486
Branches ? 0
=========================================
Hits ? 13380
Misses ? 1106
Partials ? 0 Continue to review full report at Codecov.
|
@jakob-r approved? |
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.
I am on it
Ok - i just did a commit how I would do it. If you dislike it feel free to revert to another solution |
Thanks. I am not an expert in advanced Param specifications so I'll take whatever works :) |
* Refactor function calls from packages (`<pkg::fun>`) within ParamSets (mlr-org#2730)
fixes #2729
We cannot have
pkg::<fun>
call within the ParamSets of learners as they will causelistLearners()
to fail if the learner package is not installed.