refactor: name variables after what they mean (#197)

* `FSelectorBatchSequential`: `x`/`y`/`z` communicated nothing and the
  inner `if` re-tested the condition `z` already filtered on. The scalar
  `ifelse()` calls are replaced by a single `add_feature` flag.
* `AutoFSelector`: the two near-identical row id checks are folded into
  one loop that uses `$train_set()` / `$test_set()`.
* `AutoFSelector`: `stopf("Learner ''%s' cannot calculate important
  scores.")` had a doubled quote and said "important" instead of
  "importance".
* `mlr_callbacks`: `load_callback_one_se_rule()` assigned to a variable
  that is never read, unlike its four sibling loaders.
* The `({ ... })` wrapper around the `repeat` body has no effect.

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: `AutoFSelector$train()` did not check the row ids of an instantiated inner resampling for cross-validation and reported a wrong set number for holdout (#197).
 * fix: The `mlr3fselect.svm_rfe` callback accepted support vector machines without a `type` or `kernel` setting, although only `type = "C-classification"` and `kernel = "linear"` are supported. The callback now also errors on multi-class tasks for which the importance scores are not defined (#173).
 * fix: The asynchronous feature selection ignored the `always_included` column role. Columns with this role were excluded from the models instead of being added to every feature subset (#175).
 * 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).

R/AutoFSelector.R

@@ -188,7 +188,7 @@ AutoFSelector = R6Class(
     #' @return Named `numeric()`.
     importance = function() {
       if ("importance" %nin% self$instance_args$learner$properties) {
-        stopf("Learner ''%s' cannot calculate important scores.", self$instance_args$learner$id)
+        stopf("Learner '%s' cannot calculate importance scores.", self$instance_args$learner$id)
       }
       if (is.null(self$model$learner$model)) {
         self$instance_args$learner$importance()
@@ -204,7 +204,7 @@ AutoFSelector = R6Class(
     #' @return `character()`.
     selected_features = function() {
       if ("selected_features" %nin% self$instance_args$learner$properties) {
-        stopf("Learner ''%s' cannot select features.", self$instance_args$learner$id)
+        stopf("Learner '%s' cannot select features.", self$instance_args$learner$id)
       }
       if (is.null(self$model$learner$model)) {
         self$instance_args$learner$selected_features()
@@ -349,29 +349,22 @@ AutoFSelector = R6Class(
       ia$task = task$clone()
 
       # check if task contains all row ids required for instantiated resampling
+      # `$train_set()` and `$test_set()` are used because the layout of `$instance` differs between resamplings
       if (ia$resampling$is_instantiated) {
-        imap(ia$resampling$instance$train, function(x, i) {
-          if (!test_subset(x, task$row_ids)) {
-            stopf(
-              "Train set %i of inner resampling '%s' contains row ids not present in task '%s': {%s}",
-              i,
-              ia$resampling$id,
-              task$id,
-              paste(setdiff(x, task$row_ids), collapse = ", ")
-            )
-          }
-        })
-
-        imap(ia$resampling$instance$test, function(x, i) {
-          if (!test_subset(x, task$row_ids)) {
-            stopf(
-              "Test set %i of inner resampling '%s' contains row ids not present in task '%s': {%s}",
-              i,
-              ia$resampling$id,
-              task$id,
-              paste(setdiff(x, task$row_ids), collapse = ", ")
-            )
-          }
+        walk(seq_len(ia$resampling$iters), function(i) {
+          sets = list(Train = ia$resampling$train_set(i), Test = ia$resampling$test_set(i))
+          imap(sets, function(row_ids, set_type) {
+            if (!test_subset(row_ids, task$row_ids)) {
+              stopf(
+                "%s set %i of inner resampling '%s' contains row ids not present in task '%s': {%s}",
+                set_type,
+                i,
+                ia$resampling$id,
+                task$id,
+                paste(setdiff(row_ids, task$row_ids), collapse = ", ")
+              )
+            }
+          })
         })
       }
 

R/FSelectorBatchSequential.R

@@ -94,30 +94,27 @@ FSelectorBatchSequential = R6Class(
 
       inst$eval_batch(states)
 
-      repeat {
-        ({
-          if (archive$n_batch == pars$max_features - pars$min_features + 1) {
-            break
-          }
+      # forward selection adds a feature to the best set, backward selection removes one
+      add_feature = pars$strategy == "sfs"
 
-          res = archive$best(batch = archive$n_batch)
-          best_state = as.logical(res[, feature_names, with = FALSE])
+      repeat {
+        if (archive$n_batch == pars$max_features - pars$min_features + 1) {
+          break
+        }
 
-          # Generate new states based on best feature set
-          x = ifelse(pars$strategy == "sfs", FALSE, TRUE)
-          y = ifelse(pars$strategy == "sfs", TRUE, FALSE)
-          z = if (pars$strategy == "sfs") !best_state else best_state
+        res = archive$best(batch = archive$n_batch)
+        best_state = as.logical(res[, feature_names, with = FALSE])
 
-          states = map_dtr(seq_along(best_state)[z], function(i) {
-            if (best_state[i] == x) {
-              new_state = best_state
-              new_state[i] = y
-              set_names(as.list(new_state), feature_names)
-            }
-          })
+        # generate new states by flipping one feature of the best feature set
+        candidates = if (add_feature) which(!best_state) else which(best_state)
 
-          inst$eval_batch(states)
+        states = map_dtr(candidates, function(i) {
+          new_state = best_state
+          new_state[i] = add_feature
+          set_names(as.list(new_state), feature_names)
         })
+
+        inst$eval_batch(states)
       }
     }
   )

R/mlr_callbacks.R

@@ -172,7 +172,7 @@ load_callback_svm_rfe = function() {
 NULL
 
 load_callback_one_se_rule = function() {
-  callback = callback_batch_fselect(
+  callback_batch_fselect(
     "mlr3fselect.one_se_rule",
     label = "One Standard Error Rule Callback",
     man = "mlr3fselect::mlr3fselect.one_se_rule",

tests/testthat/test_AutoFSelector.R

@@ -284,3 +284,23 @@ test_that("AutoFSelector works with async fselector", {
   expect_data_table(at$fselect_instance$result, nrows = 1)
   expect_data_table(at$fselect_instance$archive$data, min.rows = 4)
 })
+
+test_that("instantiated resampling with foreign row ids is rejected", {
+  run = function(key, ...) {
+    resampling = rsmp(key, ...)
+    resampling$instantiate(tsk("iris"))
+    at = AutoFSelector$new(
+      fselector = fs("random_search", batch_size = 1),
+      learner = lrn("classif.rpart"),
+      resampling = rsmp(key, ...),
+      measure = msr("classif.ce"),
+      terminator = trm("evals", n_evals = 2)
+    )
+    # the constructor rejects instantiated resamplings, so the check is only reachable via `$instance_args`
+    at$instance_args$resampling = resampling
+    at$train(tsk("iris")$filter(1:50))
+  }
+
+  expect_error(run("cv", folds = 3), "set 1 of inner resampling 'cv' contains row ids")
+  expect_error(run("holdout"), "set 1 of inner resampling 'holdout' contains row ids")
+})