Merge branch 'main' into fix/one-se-rule

Author: be-marc

NEWS.md

@@ -2,6 +2,11 @@
 
 * fix: `ArchiveAsyncFSelect` pushed results with the removed `rush::Rush$push_results()` method.
 * fix: The `mlr3fselect.one_se_rule` callback errored on archives with a single evaluation or with missing scores, and wrote the `n_features` column as a list column instead of an integer column (#174).
+* fix: `extract_inner_fselect_results()` added the `iteration` and `fselect_instance` columns to the result of the inner `FSelectInstance` by reference, which created a circular reference between the instance and its own result (#172).
+* fix: `fs("rfe")` and `fs("rfecv")` failed with an internal `data.table` error when `store_benchmark_result = FALSE` was set because the importance scores were read from the benchmark result of the archive (#169).
+* 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).
 
 # mlr3fselect 1.6.0
 

R/FSelectorBatchRFE.R

@@ -210,6 +210,17 @@ rank_importance = function(learners, features) {
   sort(set_names(pmap_dbl(ranked_importances, function(...) mean(c(...))), names(importances[[1]])), decreasing = TRUE)
 }
 
+# Calculates the importance scores of the feature subsets evaluated in the last batch.
+# The benchmark result of the objective is used instead of the one of the archive because it holds the models of the
+# last batch even if `store_benchmark_result` is `FALSE`.
+batch_importances = function(inst, aggregation) {
+  benchmark_result = get_private(inst$objective)$.benchmark_result
+  map(benchmark_result$uhashes, function(uhash) {
+    rr = benchmark_result$resample_result(uhash = uhash)
+    aggregation(rr$learners, rr$task$feature_names)
+  })
+}
+
 # Returns the sizes of the feature subsets
 rfe_subsets = function(n, n_features, feature_number, subset_sizes, feature_fraction) {
   subsets = if (!is.null(feature_number)) {
@@ -240,11 +251,7 @@ rfe_workhorse = function(inst, subsets, recursive, aggregation = raw_importance,
   inst$eval_batch(states)
 
   # Calculate the variable importance on the full feature set
-  uhashes = archive$data[list(archive$n_batch), "uhash", on = "batch_nr"][[1]]
-  importances = map(uhashes, function(uhash) {
-    rr = archive$benchmark_result$resample_result(uhash = uhash)
-    aggregation(rr$learners, rr$task$feature_names)
-  })
+  importances = batch_importances(inst, aggregation)
 
   # discard models if requested by the user
   if (!inst$objective$store_models) {
@@ -267,17 +274,18 @@ rfe_workhorse = function(inst, subsets, recursive, aggregation = raw_importance,
 
     if (recursive) {
       # recalculate the variable importance on the reduced feature subset
-      uhashes = archive$data[list(archive$n_batch), "uhash", on = "batch_nr"][[1]]
-      importances = map(uhashes, function(uhash) {
-        rr = archive$benchmark_result$resample_result(uhash = uhash)
-        aggregation(rr$learners, rr$task$feature_names)
-      })
+      importances = batch_importances(inst, aggregation)
 
       # log importance to archive
       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) {

R/FSelectorBatchRFECV.R

@@ -165,7 +165,8 @@ FSelectorBatchRFECV = R6Class(
 
       # average performance of feature numbers
       aggr = archive$data[, list("y" = mean(unlist(.SD))), by = "batch_nr", .SDcols = archive$cols_y]
-      best_batch = aggr[order(get("y"), decreasing = TRUE), head(.SD, 1)]$batch_nr
+      # the direction of the codomain determines whether the performance is minimized or maximized
+      best_batch = aggr[which_max(aggr[["y"]] * -archive$codomain$direction, ties_method = "first")]$batch_nr
       n_features = rowSums(archive$data[list(best_batch), , on = "batch_nr"][1, archive$cols_x, with = FALSE])
 
       # use full data set

R/extract_inner_fselect_results.R

@@ -63,7 +63,8 @@ extract_inner_fselect_results.ResampleResult = function(x, fselect_instance = FA
     return(data.table())
   }
   tab = imap_dtr(rr$learners, function(learner, i) {
-    data = setalloccol(learner$fselect_result)
+    # copy the result, otherwise the columns below are added to the result of the instance
+    data = copy(learner$fselect_result)
     set(data, j = "iteration", value = i)
     if (fselect_instance) {
       set(data, j = "fselect_instance", value = list(learner$fselect_instance))

tests/testthat/test_FSelectorRFE.R

@@ -319,3 +319,23 @@ test_that("optimal features are selected with mean", {
 
   expect_equal(instance$result$features[[1]], c("x2", "x3", "x4"))
 })
+
+test_that("rfe works without storing the benchmark result", {
+  instance = fsi(
+    task = TEST_MAKE_TSK(),
+    learner = lrn("regr.rpart"),
+    resampling = rsmp("cv", folds = 3),
+    measures = msr("dummy"),
+    terminator = trm("none"),
+    store_benchmark_result = FALSE
+  )
+
+  optimizer = fs("rfe", n_features = 1, feature_number = 1)
+  optimizer$optimize(instance)
+  data = instance$archive$data
+
+  expect_names(names(data), disjunct.from = "uhash")
+  expect_feature_number(data[batch_nr == 1, 1:4], n = 4)
+  expect_feature_number(data[batch_nr == 4, 1:4], n = 1)
+  pwalk(data, function(x1, x2, x3, x4, importance, ...) expect_equal(x1 + x2 + x3 + x4, length(importance)))
+})

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")
@@ -85,6 +110,53 @@ test_that("learner without importance method throw an error", {
   )
 })
 
+test_that("optimal features are selected with a minimizing measure", {
+  LearnerRegrDebugImportance = R6Class(
+    "LearnerRegrDebugImportance",
+    inherit = LearnerRegrDebug,
+    public = list(
+      importance = function() {
+        c(x2 = 1.4, x1 = 0.8, x3 = 1.2, x4 = 1.1)
+      }
+    )
+  )
+
+  learner = LearnerRegrDebugImportance$new()
+  learner$properties = c(learner$properties, "importance")
+
+  # the three features x2, x3 and x4 have the lowest score
+  score_design = data.table(
+    score = c(2, 1, 4, 3),
+    features = list(
+      c("x1", "x2", "x3", "x4"),
+      c("x2", "x3", "x4"),
+      c("x2", "x3"),
+      "x2"
+    )
+  )
+
+  measure = msr("dummy", score_design = score_design, minimize = TRUE)
+
+  instance = fsi(
+    task = TEST_MAKE_TSK(),
+    learner = learner,
+    resampling = rsmp("cv", folds = 3),
+    measures = measure,
+    terminator = trm("none"),
+    store_models = TRUE
+  )
+
+  optimizer = fs("rfecv", n_features = 1, feature_number = 1)
+  optimizer$optimize(instance)
+  data = as.data.table(instance$archive)
+
+  # number of features in the final run
+  expect_feature_number(data[13, 1:4], n = 4)
+  expect_feature_number(data[14, 1:4], n = 3)
+
+  expect_equal(instance$result$features[[1]], c("x2", "x3", "x4"))
+})
+
 test_that("optimal features are selected", {
   LearnerRegrDebugImportance = R6Class(
     "LearnerRegrDebugImportance",
@@ -150,3 +222,22 @@ test_that("optimal features are selected", {
 
   expect_equal(instance$result$features[[1]], c("x2", "x3", "x4"))
 })
+
+test_that("rfecv works without storing the benchmark result", {
+  instance = fsi(
+    task = TEST_MAKE_TSK(),
+    learner = lrn("regr.rpart"),
+    resampling = rsmp("cv", folds = 3),
+    measures = msr("dummy"),
+    terminator = trm("none"),
+    store_benchmark_result = FALSE
+  )
+
+  optimizer = fs("rfecv", n_features = 1, feature_number = 1)
+  optimizer$optimize(instance)
+  data = instance$archive$data
+
+  expect_names(names(data), disjunct.from = "uhash")
+  expect_names(names(data), must.include = c("importance", "iteration"))
+  pwalk(data, function(x1, x2, x3, x4, importance, ...) expect_equal(x1 + x2 + x3 + x4, length(importance)))
+})

tests/testthat/test_extract_inner_fselect_result.R

@@ -377,3 +377,27 @@ test_that("extract_inner_fselect_results function works with benchmark and retur
   )
   expect_equal(unique(ibmr$experiment), c(1, 2))
 })
+
+test_that("extract_inner_fselect_results does not modify the fselect result", {
+  rr = fselect_nested(
+    fs("random_search"),
+    tsk("iris"),
+    lrn("classif.rpart"),
+    rsmp("holdout"),
+    rsmp("cv", folds = 2),
+    msr("classif.ce"),
+    term_evals = 4
+  )
+
+  columns = names(rr$learners[[1]]$fselect_result)
+
+  extract_inner_fselect_results(rr)
+  expect_named(rr$learners[[1]]$fselect_result, columns)
+
+  extract_inner_fselect_results(rr, fselect_instance = TRUE)
+  expect_named(rr$learners[[1]]$fselect_result, columns)
+
+  # the instance is not added again when it is not requested
+  irr = extract_inner_fselect_results(rr)
+  expect_names(names(irr), disjunct.from = "fselect_instance")
+})