PipeOpLearnerCVPlus #838
PipeOpLearnerCVPlus #838
Conversation
There was a problem hiding this comment.
I think the reasons why the tests fail is that lrn("regr.ranger") is not in the lrn() dictionary when mlr3learners is not loaded. So you either have to load that, or see if you can use lrn("regr.rpart") instead, which is present when using mlr3 without mlr3learners.
R/PipeOpLearnerCVPlus.R
Outdated
| } | ||
|
|
||
| task_type = mlr_reflections$task_types[type, mult = "first"]$task | ||
| out_type = mlr_reflections$task_types[type, mult = "first"]$task |
There was a problem hiding this comment.
I don't think you need out_type here? You can just use the task_type variable.
There was a problem hiding this comment.
out_type was necessary here, but I changed it to mlr_reflections$task_types[type, mult = "first"]$prediction like you suggested.
R/PipeOpLearnerCVPlus.R
Outdated
| prds = rbindlist(map(rr$predictions(predict_sets = "test"), as.data.table), idcol = "fold") | ||
|
|
||
| # Add trained models and residuals to PipeOp state | ||
| self$state = list(cv_models = rr$learners, |
There was a problem hiding this comment.
| self$state = list(cv_models = rr$learners, | |
| self$state = list(cv_model_states = map(rr$learners, "state"), |
There was a problem hiding this comment.
It is best to only store the essential information in the PipeOp's $state, which are the model states. Try not to store the entire learner objects.
There was a problem hiding this comment.
You'd also need to update the documentation.
There was a problem hiding this comment.
Okay, I changed the state and documentation to only include the state.
R/PipeOpLearnerCVPlus.R
Outdated
| if (is.null(self$state) || is_noop(self$state)) { | ||
| private$.learner | ||
| } else { | ||
| multiplicity_recurse(self$state, clone_with_state, learner = private$.learner) |
There was a problem hiding this comment.
this does not work, since the PipeOp's $state is not the same as the Learner's $state; this is different in the PipeOpLearnerCV. But tbh I don't know what kind of result one would want here, maybe best to discuss this.
One possibility of what could work would be
multiplicity_recurse(self$state, learner = function(state) {
map(state$cv_model_states, clone_with_state, learner = private$.learner)
})There was a problem hiding this comment.
This assumes you accept the 'cv_model_states' edit below. Also I haven't checked it, there may be a typo in there.
R/PipeOpLearnerCVPlus.R
Outdated
| mu_hat = map(self$state$cv_models, function(learner) { | ||
| as.data.table(learner$predict(task)) | ||
| }) |
There was a problem hiding this comment.
| mu_hat = map(self$state$cv_models, function(learner) { | |
| as.data.table(learner$predict(task)) | |
| }) | |
| mu_hat = map(self$state$cv_model_states, function(state) { | |
| on.exit({private$.learner$state = NULL}) | |
| private$.learner$state = state | |
| as.data.table(private$.learner$predict(task)) | |
| }) |
|
Also don't forget to add a NEWS entry. And feel free to add yourself as |
No description provided.