codex code review

Author: mb706

NEWS.md

@@ -6,6 +6,8 @@
 * Added support for internal validation tasks to `PipeOpFeatureUnion`.
 * feat: `PipeOpLearnerCV` can reuse the cross-validation models during prediction by averaging their outputs (`resampling.predict_method = "cv_ensemble"`).
 * feat: `PipeOpRegrAvg` gets new `se_aggr` and `se_aggr_rho` hyperparameters and now allows various forms of SE aggregation.
+* Fix: `PipeOpInfo` now prints a bounded task preview (respecting target/feature ordering and row ids) and collapses logger output to single messages.
+* Fix: `PipeOpIsomap` only operates on numeric or integer features and its parameter documentation was corrected.
 
 # mlr3pipelines 0.9.0
 

R/PipeOpInfo.R

@@ -82,7 +82,13 @@ PipeOpInfo = R6Class("PipeOpInfo",
       )
       original_printer = list(
         Task = crate(function(x) {
-          list(task = x, data = x$data()[, 1:min(10, ncol(x$data()))])
+          row_preview = head(x$row_ids, 10L)
+          col_preview = head(c(x$target_names, x$feature_names), 10L)
+          data_preview = x$data(rows = row_preview, cols = col_preview)
+          list(
+            task = x,
+            data_preview = data_preview
+          )
         }),
         Prediction = crate(function(x) {
           tryCatch(list(prediction = x, score = x$score()), error = function(e) {list(prediction = x)})
@@ -108,7 +114,7 @@ PipeOpInfo = R6Class("PipeOpInfo",
     .printer = NULL,
     .log_target = NULL,
     .output = function(inputs, stage) {
-    input_class = class(inputs[[1]])
+      input_class = class(inputs[[1]])
       leftmost_class =
         if (any(input_class %in% names(private$.printer))) {
           input_class[input_class %in% names(private$.printer)][[1]]
@@ -125,19 +131,20 @@ PipeOpInfo = R6Class("PipeOpInfo",
         cat(stage_string, "\n\n")
         specific_printer(inputs[[1]])
       })
+      message_text = paste(print_string, collapse = "\n")
       if (log_target_split[[1]] == "lgr") {
         logger = lgr::get_logger(log_target_split[[2]])
         log_level = log_target_split[[3]]
-        logger$log(log_level, msg = print_string)
+        logger$log(log_level, msg = message_text)
       } else if (private$.log_target == "cat") {
-        cat(paste(print_string, collapse = "\n"))
+        cat(message_text)
       } else if (private$.log_target == "message") {
-        message(paste(print_string, collapse = "\n"))
+        message(message_text)
       } else if (private$.log_target == "warning") {
-        warning(paste(print_string, collapse = "\n"))
+        warning(message_text)
       } else if (private$.log_target == "none") {
       } else {
-        stopf("Invalid log_target '%s'.", log_target)
+        stopf("Invalid log_target '%s'.", private$.log_target)
       }
     },
     .train = function(inputs, stage = "Training") {

R/PipeOpIsomap.R

@@ -33,18 +33,18 @@
 #'
 #' @section Parameters:
 #' The parameters are the parameters inherited from [`PipeOpTaskPreproc`], as well as:
-#' * `knn` :: `numeric(1)`\cr
+#' * `knn` :: `integer(1)`\cr
 #'   The number of nearest neighbors in the graph.
 #'   Initialized to 50.
-#' * `ndim` :: `numeric(1)`\cr
+#' * `ndim` :: `integer(1)`\cr
 #'   The number of embedding dimensions.
 #'   Initialized to 2.
 #' * `get_geod` :: `logical(1)`\cr
 #'   Determines whether the distance matrix should be kept in the `$state`.
 #'   Initialized to `FALSE`.
 #' * `.mute` :: `character`\cr
 #'   A character vector of elements to mute during training (e.g. c("message", "output")).
-#'   Default: `character(0)`.
+#'   Initialized to `NULL`.
 #'
 #' @section Internals:
 #' Applies the Isomap embedding from the `dimRed`-package.
@@ -78,7 +78,8 @@ PipeOpIsomap = R6Class("PipeOpIsomap",
         get_geod = p_lgl(default = FALSE, tags = c("train", "isomap")),
         .mute = p_uty(init = NULL, tags = c("train", "isomap"))
       )
-      super$initialize(id = id, param_set = ps, param_vals = param_vals, packages = c("dimRed", "stats"))
+      super$initialize(id = id, param_set = ps, param_vals = param_vals,
+        packages = c("dimRed", "stats"), feature_types = c("numeric", "integer"))
     }
   ),
   private = list(

tests/testthat/test_pipeop_info.R

@@ -66,10 +66,12 @@ test_that("logger is addressed when log_target is set to a logger", {
   for (j in input) {
     poinfo$train(list(j))
     logfile_posttrain = appender_buffer$data$msg
+    expect_equal(length(logfile_posttrain), 1L)
     expect_match(logfile_posttrain, "Object passing through PipeOp info - Training", all = FALSE)
     appender_buffer$flush()
     poinfo$predict(list(j))
     logfile_postprediction = appender_buffer$data$msg
+    expect_equal(length(logfile_postprediction), 1L)
     expect_match(logfile_postprediction, "Object passing through PipeOp info - Prediction", all = FALSE)
     appender_buffer$flush()
   }
@@ -103,6 +105,19 @@ test_that("PipeOp recognizes class of input objects and prints information accor
   }
 })
 
+test_that("Task printer only displays a preview", {
+  task = tsk("iris")
+  poinfo = po("info")
+  preview = poinfo$printer$Task(task)
+  row_preview = head(task$row_ids, 10L)
+  col_preview = head(c(task$target_names, task$feature_names), 10L)
+  expect_true(is.list(preview))
+  expect_identical(names(preview), c("task", "data_preview"))
+  expect_identical(preview$task, task)
+  expect_equal(preview$data_preview,
+    task$data(rows = row_preview, cols = col_preview))
+})
+
 test_that("malformed log_target handled accordingly", {
   malformed_log_target = list("malformed", "::", "::::::", "log::", "log::log_level", "log::log_level::", "log::log_level::message::", "::log")
   for (i in seq_along(malformed_log_target)) {

tests/testthat/test_pipeop_isomap.R

@@ -2,6 +2,7 @@ context("PipeOpIsomap")
 
 test_that("PipeOpIsomap - basic properties", {
   skip_if_not_installed("dimRed")
+  skip_if_not_installed("RSpectra")
   skip_if_not_installed("stats")
   op = po("isomap", .mute = "message")
   task = mlr_tasks$get("iris")
@@ -11,6 +12,7 @@ test_that("PipeOpIsomap - basic properties", {
 
 test_that("compare to dimRed::isomap", {
   skip_if_not_installed("dimRed")
+  skip_if_not_installed("RSpectra")
   skip_if_not_installed("stats")
   # Part 1 - Train-method
   # Version 1 - PipeOpIsomap
@@ -40,15 +42,35 @@ test_that("compare to dimRed::isomap", {
 
 test_that("isomap algorithm requires data to be numeric", {
   skip_if_not_installed("dimRed")
+  skip_if_not_installed("RSpectra")
   skip_if_not_installed("stats")
   po = po("isomap")
   task = tsk("penguins")
   task$filter(which(complete.cases(task$data())))
   expect_error(po$train(list(task)))
 })
 
+test_that("isomap leaves non-numeric features untouched", {
+  skip_if_not_installed("dimRed")
+  skip_if_not_installed("RSpectra")
+  skip_if_not_installed("stats")
+  backend = data.table::as.data.table(tsk("iris")$data())
+  backend$species_factor = factor(rep(letters[1:3], length.out = nrow(backend)))
+  task = mlr3::TaskClassif$new("iris_factor", backend = backend, target = "Species")
+  po = po("isomap", ndim = 3)
+  trained_task = po$train(list(task))[[1]]
+  predicted_task = po$predict(list(task))[[1]]
+
+  expect_true("species_factor" %in% trained_task$feature_names)
+  expect_equal(trained_task$data(cols = "species_factor"), task$data(cols = "species_factor"))
+  expect_equal(predicted_task$data(cols = "species_factor"), task$data(cols = "species_factor"))
+  expect_equal(sum(grepl("^iso ", trained_task$feature_names)), 3L)
+  expect_equal(sum(grepl("^iso ", predicted_task$feature_names)), 3L)
+})
+
 test_that("hyperparameter ndim", {
   skip_if_not_installed("dimRed")
+  skip_if_not_installed("RSpectra")
   skip_if_not_installed("stats")
   for (i in seq_len(length(tsk("iris")$feature_names))) {
     po = po("isomap", ndim = i)
@@ -59,6 +81,7 @@ test_that("hyperparameter ndim", {
 
 test_that("hyperparameter get_geod", {
   skip_if_not_installed("dimRed")
+  skip_if_not_installed("RSpectra")
   skip_if_not_installed("stats")
   # Check 1 - get_geod = FALSE behaves as expected
   po_no_geod = po("isomap", get_geod = FALSE)
@@ -76,6 +99,7 @@ test_that("hyperparameter get_geod", {
 
 test_that("hyperparameter .mute", {
   skip_if_not_installed("dimRed")
+  skip_if_not_installed("RSpectra")
   skip_if_not_installed("stats")
   po = po("isomap", .mute = c("message", "output"))
 #  expect_silent(po$train(list(tsk("iris")))) # does not work because of testthat #1480