fix(rfe): write importance to the whole batch when recursive is FALSE (#168)

Author: be-marc

NEWS.md

@@ -1,6 +1,7 @@
 # mlr3fselect (development version)
 
 * fix: `ArchiveAsyncFSelect` pushed results with the removed `rush::Rush$push_results()` method.
+* fix: `fs("rfecv", recursive = FALSE)` failed with an internal `data.table` error because the importance scores of all resampling iterations were written to a single archive row (#168).
 * fix: `fs("rfecv")` ignored the direction of the measure and selected the feature set size with the worst mean performance for minimizing measures such as `msr("classif.ce")` or `msr("regr.mse")`.
   Feature selection results obtained with `fs("rfecv")` and a minimizing measure are invalid and should be recomputed (#167).
 

R/FSelectorBatchRFE.R

@@ -277,7 +277,12 @@ rfe_workhorse = function(inst, subsets, recursive, aggregation = raw_importance,
       archive$data[list(archive$n_batch), "importance" := importances, on = "batch_nr"]
     } else {
       # log importance to archive
-      set(archive$data, archive$n_evals, "importance", map(importances, function(x) x[seq(j)]))
+      # the batch holds one row per fold, so the truncated importance of each fold is assigned to the whole batch
+      archive$data[
+        list(archive$n_batch),
+        "importance" := map(importances, function(x) x[seq(j)]),
+        on = "batch_nr"
+      ]
     }
   }
   if (folds > 1) {

tests/testthat/test_FSelectorRFECV.R

@@ -68,6 +68,31 @@ test_that("default parameters work", {
   })
 })
 
+test_that("recursive parameter works", {
+  instance = fsi(
+    task = TEST_MAKE_TSK(),
+    learner = lrn("regr.rpart"),
+    resampling = rsmp("cv", folds = 3),
+    measures = msr("dummy"),
+    terminator = trm("none"),
+    store_models = TRUE
+  )
+
+  optimizer = fs("rfecv", recursive = FALSE, n_features = 1, feature_number = 1)
+  optimizer$optimize(instance)
+  data = instance$archive$data
+
+  # the importance of the first batch is reused and truncated in the following batches
+  walk(seq(3), function(i) {
+    importances = data[list(i), importance, on = "iteration"]
+    walk(seq(2, length(importances)), function(j) {
+      expect_equal(importances[[j]], importances[[1]][seq(length(importances) + 1 - j)])
+    })
+  })
+
+  pwalk(data, function(x1, x2, x3, x4, importance, ...) expect_equal(x1 + x2 + x3 + x4, length(importance)))
+})
+
 test_that("learner without importance method throw an error", {
   learner = lrn("classif.rpart")
   learner$properties = setdiff(learner$properties, "importance")