Merge pull request #702 from mlr-org/id_clashes

feature: allow id suffixes when retrieving dictionary element

NEWS.md

@@ -1,4 +1,5 @@
 # mlr3pipelines 0.4.2-9000
+* `po()`, `pos()` can now construct `PipeOp`s with ID postfix `_<number>` to avoid ID clashes.
 
 # mlr3pipelines 0.4.2
 

R/mlr_pipeops.R

@@ -48,6 +48,7 @@ mlr_pipeops = R6Class("DictionaryPipeOp", inherit = mlr3misc::Dictionary,
   public = list(
     metainf = new.env(parent = emptyenv()),
     add = function(key, value, metainf = NULL) {
+      assert_false(grepl("_\\d+$", key))
       ret = super$add(key, value)
       if (!is.null(metainf)) {
         # we save the *expression*, not the value, because we could otherwise get version conflicts from objects.

R/po.R

@@ -82,7 +82,7 @@ po.PipeOp = function(.obj, ...) {
 
 #' @export
 po.character = function(.obj, ...) {
-  dictionary_sugar_get(dict = mlr_pipeops, .key = .obj, ...)
+  dictionary_sugar_inc_get(dict = mlr_pipeops, .key = .obj, ...)
 }
 
 #' @export
@@ -110,7 +110,7 @@ pos.NULL = function(.objs, ...) {
 
 #' @export
 pos.character = function(.objs, ...) {
-  dictionary_sugar_mget(dict = mlr_pipeops, .keys = .objs, ...)
+  dictionary_sugar_inc_mget(dict = mlr_pipeops, .keys = .objs, ...)
 }
 
 #' @export

R/utils.R

@@ -103,3 +103,34 @@ multiplicity_recurse = function(.multip, .fun, ...) {
     .fun(.multip, ...)
   }
 }
+
+# replace when new mlr3misc version is released https://github.com/mlr-org/mlr3misc/pull/80
+dictionary_sugar_inc_get = function(dict, .key, ...) {
+  newkey = gsub("_\\d+$", "", .key)
+  add_suffix = .key != newkey
+  if (add_suffix) {
+    assert_true(!methods::hasArg("id"))
+    suffix = regmatches(.key, regexpr("_\\d+$", .key))
+  }
+  obj = mlr3misc::dictionary_sugar_get(dict = dict, .key = newkey, ...)
+
+  if (add_suffix) {
+    obj$id = paste0(obj$id, suffix)
+  }
+  obj
+
+}
+
+# replace when new mlr3misc version is released https://github.com/mlr-org/mlr3misc/pull/80
+dictionary_sugar_inc_mget = function(dict, .keys, ...) {
+  objs = lapply(.keys, dictionary_sugar_inc_get, dict = dict, ...)
+  if (!is.null(names(.keys))) {
+    nn = names2(.keys)
+    ii = which(!is.na(nn))
+    for (i in ii) {
+      objs[[i]]$id = nn[i]
+    }
+  }
+  names(objs) = map_chr(objs, "id")
+  objs
+}

pkgdown/_pkgdown.yml

@@ -83,6 +83,7 @@ reference:
       - filter_noop
       - starts_with("as.")
       - starts_with("is.")
+      - starts_with("is.")
   - title: Abstract PipeOps
     contents:
       - PipeOpEnsemble

tests/testthat/test_dictionary.R

@@ -211,3 +211,8 @@ test_that("mlr_graphs dictionary", {
   expect_data_table(dt, col.names = "unique")
   expect_true("key" %in% colnames(dt))
 })
+
+test_that("Cannot add pipeops with keys that invalidates the convenience for id incrementation", {
+  copy = mlr_pipeops$clone(deep = TRUE)
+  expect_error(copy$add("name_1", PipeOp), regexp = "grepl")
+})

tests/testthat/test_po.R

@@ -202,3 +202,16 @@ test_that("mlr_pipeops multi-access works", {
   )
 
 })
+
+test_that("Incrementing ids works", {
+  x = po("pca_123")
+  expect_true(x$id == "pca_123")
+  expect_r6(x, "PipeOpPCA")
+
+  x = po("learner_1", lrn("regr.rpart"))
+  expect_true(x$id == "regr.rpart_1")
+  expect_r6(x, "PipeOpLearner")
+
+  xs = pos(c("pca_1", "pca_2"))
+  assert_true(all(names(xs) == c("pca_1", "pca_2")))
+})