From 37980bcae5b06a94104444567f15d6d4bead3e5c Mon Sep 17 00:00:00 2001 From: Jakob Richter Date: Thu, 11 Aug 2016 13:55:40 +0200 Subject: [PATCH 1/8] Fix: Defaults get handled more reliable --- R/updateParVals.R | 6 +++++- tests/testthat/test_isFeasible.R | 8 ++++++++ tests/testthat/test_updateParVals.R | 14 ++++++++++++++ 3 files changed, 27 insertions(+), 1 deletion(-) diff --git a/R/updateParVals.R b/R/updateParVals.R index 7e9f28ba..4aebfa6b 100644 --- a/R/updateParVals.R +++ b/R/updateParVals.R @@ -17,6 +17,10 @@ updateParVals = function(par.set, old.par.vals, new.par.vals, warn = FALSE) { assertList(old.par.vals, names = "named") assertList(new.par.vals, names = "named") assertClass(par.set, "ParamSet") + + #we might want to check requires with defaults that are not overwritten by new.par.vals + usable.defaults = getDefaults(par.set) + usable.defaults = usable.defaults[names(usable.defaults) %nin% names(new.par.vals)] repeat { # we repeat to include parameters which depend on each other by requirements # candidates are params of the old par.vals we might still need. @@ -25,7 +29,7 @@ updateParVals = function(par.set, old.par.vals, new.par.vals, warn = FALSE) { # If all requirement parameters for the candidate are in the new.par.vals and if the requirements are met if (all(getRequiredParamNames(par.set$pars[[pn]]) %in% names(new.par.vals)) && requiresOk(par.set$pars[[pn]], new.par.vals)) { new.par.vals[pn] = old.par.vals[pn] # keep old.par.val in new.par.vals as it meets the requirements - } else if (all(getRequiredParamNames(par.set$pars[[pn]]) %in% names(getDefaults(par.set))) && requiresOk(par.set$pars[[pn]], getDefaults(par.set))) { + } else if (all(getRequiredParamNames(par.set$pars[[pn]]) %in% names(usable.defaults)) && requiresOk(par.set$pars[[pn]], usable.defaults)) { new.par.vals[pn] = old.par.vals[pn] # keep old.par.val as it meets requirement via defaults } else if (warn){ # otherwise we can drop the old par.val because it does not meet the requirements. diff --git a/tests/testthat/test_isFeasible.R b/tests/testthat/test_isFeasible.R index afc72ef3..3f332598 100644 --- a/tests/testthat/test_isFeasible.R +++ b/tests/testthat/test_isFeasible.R @@ -23,6 +23,14 @@ test_that("isFeasible ParamSet", { expect_match(attr(res, "warning"), "Param c=2 is set but does not meet requirements") expect_true((res = isFeasible(ps, list(a = 2, c = NA), filter = TRUE))) expect_true(isFeasible(ps, list(c = 2), filter = TRUE, use.defaults = TRUE)) + + #make sure we can ignore defaults if they contradict a new setting + ps = makeParamSet( + makeIntegerParam("a", default = 1L, requires = quote(b == 2)), + makeIntegerParam("b", default = 2L, requires = quote(c == TRUE)), + makeLogicalParam("c", default = TRUE)) + expect_error(isFeasible(ps, list(b = 3), filter = TRUE), "needed for requirements: c") + expect_true(isFeasible(ps, list(b = 3), filter = TRUE, use.defaults = TRUE)) }) test_that("isFeasible LearnerParamSet", { diff --git a/tests/testthat/test_updateParVals.R b/tests/testthat/test_updateParVals.R index 31d83ec2..8c50341c 100644 --- a/tests/testthat/test_updateParVals.R +++ b/tests/testthat/test_updateParVals.R @@ -15,6 +15,10 @@ test_that("updateParVals works", { expect_equal(pc, list(a = 0, c = 3, d = 4, e = 5)) expect_warning(updateParVals(ps, pa, pb, warn = TRUE), "ParamSetting b=2") + pb2 = list(a = 0, f = FALSE) + pc2 = updateParVals(ps, pa, pb2) + expect_equal(pc2, list(a = 0, f = FALSE, d = 4)) + #sanity expect_equal(updateParVals(ps, pa, list()), pa) expect_equal(updateParVals(ps, list(), pb), pb) @@ -30,4 +34,14 @@ test_that("updateParVals works", { makeLogicalParam("e", default = TRUE)) pc = updateParVals(ps, pa, pb) expect_equal(pc, list(a = -3, c = 0.5)) + + #chained dependency with defaults #maybe redundanant with pc2? + ps = makeParamSet( + makeIntegerParam("a", default = 1L, requires = quote(b == 2)), + makeIntegerParam("b", default = 2L, requires = quote(c == TRUE)), + makeLogicalParam("c", default = TRUE)) + pa = list(a = 1, b = 2, c = TRUE) + pb = list(b = 3) + pc = updateParVals(ps, pa, pb) + expect_equal(pc, list(b = 3, c = TRUE)) }) From 61151445b5e9431e25d42058ea74f670bc47db03 Mon Sep 17 00:00:00 2001 From: Jakob Richter Date: Thu, 11 Aug 2016 14:10:10 +0200 Subject: [PATCH 2/8] make documentation clearer --- R/isFeasible.R | 1 + R/updateParVals.R | 1 + 2 files changed, 2 insertions(+) diff --git a/R/isFeasible.R b/R/isFeasible.R index 0fb60087..9ee10a77 100644 --- a/R/isFeasible.R +++ b/R/isFeasible.R @@ -19,6 +19,7 @@ #' which refers to an unpassed param.) #' @param use.defaults [\code{logical(1)}]\cr #' Whether defaults of the \code{\link{Param}}/\code{\link{ParamSet}} should be used if no values are supplied. +#' If the defaults have requirements that are not met by \code{x} it will be feasible nonetheless. #' Default is \code{FALSE}. #' @param filter [\code{logical(1)}]\cr #' Whether the \code{\link{ParamSet}} should be reduced to the space of the given Param Values. diff --git a/R/updateParVals.R b/R/updateParVals.R index 4aebfa6b..077c04c5 100644 --- a/R/updateParVals.R +++ b/R/updateParVals.R @@ -2,6 +2,7 @@ #' @description #' Update the values of a given parameter setting with a new parameter setting. #' Settings that do not meet the requirements anymore will be deleted from the first given parameter setting. +#' Default values of the Param Set are respected to chek if the new param settings meet the requirements. #' #' @template arg_parset #' @param old.par.vals [\code{list}]\cr From 498ab2605fe7c6c583e7b250d1c921809be6c2ad Mon Sep 17 00:00:00 2001 From: Jakob Richter Date: Thu, 11 Aug 2016 15:27:13 +0200 Subject: [PATCH 3/8] add roxygen files --- man/isFeasible.Rd | 1 + man/updateParVals.Rd | 1 + 2 files changed, 2 insertions(+) diff --git a/man/isFeasible.Rd b/man/isFeasible.Rd index 79c66907..658a3fdd 100644 --- a/man/isFeasible.Rd +++ b/man/isFeasible.Rd @@ -21,6 +21,7 @@ which refers to an unpassed param.)} \item{use.defaults}{[\code{logical(1)}]\cr Whether defaults of the \code{\link{Param}}/\code{\link{ParamSet}} should be used if no values are supplied. +If the defaults have requirements that are not met by \code{x} it will be feasible nonetheless. Default is \code{FALSE}.} \item{filter}{[\code{logical(1)}]\cr diff --git a/man/updateParVals.Rd b/man/updateParVals.Rd index 72229036..b1b2a753 100644 --- a/man/updateParVals.Rd +++ b/man/updateParVals.Rd @@ -26,5 +26,6 @@ Default is \code{FALSE}.} \description{ Update the values of a given parameter setting with a new parameter setting. Settings that do not meet the requirements anymore will be deleted from the first given parameter setting. + Default values of the Param Set are respected to chek if the new param settings meet the requirements. } From 719d15770363f7824d878eb45719e528613bda43 Mon Sep 17 00:00:00 2001 From: Jakob Richter Date: Fri, 12 Aug 2016 13:04:09 +0200 Subject: [PATCH 4/8] add complicated test case --- tests/testthat/test_updateParVals.R | 12 ++++++++++++ 1 file changed, 12 insertions(+) diff --git a/tests/testthat/test_updateParVals.R b/tests/testthat/test_updateParVals.R index 8c50341c..023a9eec 100644 --- a/tests/testthat/test_updateParVals.R +++ b/tests/testthat/test_updateParVals.R @@ -44,4 +44,16 @@ test_that("updateParVals works", { pb = list(b = 3) pc = updateParVals(ps, pa, pb) expect_equal(pc, list(b = 3, c = TRUE)) + + #more complicated stuff + ps = makeParamSet( + makeDiscreteLearnerParam(id = "a", default = "a2", + values = c("a1", "a2", "a3"), + requires = quote(!a %in% c("a2") || b == TRUE)), + makeLogicalLearnerParam(id = "b", default = FALSE, tunable = FALSE) + ) + pa = list(a = "a1") + pb = list() + pc = updateParVals(ps, pa, pb) + expect_equal(pc, pb) }) From 5238e9543803785f159047a496f8c83b27cb3d3b Mon Sep 17 00:00:00 2001 From: Jakob Richter Date: Fri, 12 Aug 2016 22:12:56 +0200 Subject: [PATCH 5/8] overhaul updateParVals to work with defaults even in complex cases --- R/updateParVals.R | 44 +++++++++++++++++++---------- tests/testthat/test_updateParVals.R | 4 +-- 2 files changed, 31 insertions(+), 17 deletions(-) diff --git a/R/updateParVals.R b/R/updateParVals.R index 077c04c5..62674504 100644 --- a/R/updateParVals.R +++ b/R/updateParVals.R @@ -18,27 +18,41 @@ updateParVals = function(par.set, old.par.vals, new.par.vals, warn = FALSE) { assertList(old.par.vals, names = "named") assertList(new.par.vals, names = "named") assertClass(par.set, "ParamSet") - - #we might want to check requires with defaults that are not overwritten by new.par.vals - usable.defaults = getDefaults(par.set) - usable.defaults = usable.defaults[names(usable.defaults) %nin% names(new.par.vals)] + assertFlag(warn) + default.par.vals = getDefaults(par.set) + # First we extend both par.vals lists with the defaults to get the fully requirements meeting par.vals lists + old.with.defaults = updateParVals2(par.set = par.set, old.par.vals = default.par.vals, new.par.vals = old.par.vals) + new.with.defaults = updateParVals2(par.set = par.set, old.par.vals = default.par.vals, new.par.vals = new.par.vals) + updated.par.set = updateParVals2(par.set = par.set, old.par.vals = old.with.defaults, new.par.vals = new.with.defaults) + kept.old = attr(old.with.defaults, "kept") + kept.new = attr(new.with.defaults, "kept") + # Find out which parmam names were kept in both update processes + # this indicates that this was a default and we don't need it, as it is still a valid default. + both.kept = intersect(names(kept.new)[kept.new], names(kept.old)[kept.old]) + result = updated.par.set[names(updated.par.set) %nin% both.kept] + if (warn) { + # detect dropped param settings: + warningf("ParamSettings (%s) were dropped.", convertToShortString(old.par.vals[names(old.par.vals) %nin% names(result)])) + } + return(result) +} + +updateParVals2 = function(par.set, old.par.vals, new.par.vals) { + kept = setNames(logical(length(new.par.vals)), names(new.par.vals)) repeat { - # we repeat to include parameters which depend on each other by requirements - # candidates are params of the old par.vals we might still need. + # we include parameters of the old.par.vals if they meet the requirements + # we repeat because some parameters of old.par.vals might only meet the requirements after we added others. (chained requirements) candidate.par.names = setdiff(names(old.par.vals), names(new.par.vals)) for (pn in candidate.par.names) { # If all requirement parameters for the candidate are in the new.par.vals and if the requirements are met - if (all(getRequiredParamNames(par.set$pars[[pn]]) %in% names(new.par.vals)) && requiresOk(par.set$pars[[pn]], new.par.vals)) { - new.par.vals[pn] = old.par.vals[pn] # keep old.par.val in new.par.vals as it meets the requirements - } else if (all(getRequiredParamNames(par.set$pars[[pn]]) %in% names(usable.defaults)) && requiresOk(par.set$pars[[pn]], usable.defaults)) { - new.par.vals[pn] = old.par.vals[pn] # keep old.par.val as it meets requirement via defaults - } else if (warn){ - # otherwise we can drop the old par.val because it does not meet the requirements. - warningf("ParamSetting %s was dropped.", convertToShortString(old.par.vals[pn])) + if (isTRUE(try(requiresOk(par.set$pars[[pn]], c(new.par.vals, old.par.vals[pn])), silent = TRUE))) { + # keep old.par.val in new.par.vals as it meets the requirements + new.par.vals[pn] = old.par.vals[pn] + kept[pn] = TRUE } } # break if no changes were made if (identical(candidate.par.names, setdiff(names(old.par.vals), names(new.par.vals)))) break } - return(new.par.vals) -} + setAttribute(new.par.vals, "kept", kept) +} \ No newline at end of file diff --git a/tests/testthat/test_updateParVals.R b/tests/testthat/test_updateParVals.R index 023a9eec..c3a042be 100644 --- a/tests/testthat/test_updateParVals.R +++ b/tests/testthat/test_updateParVals.R @@ -13,7 +13,7 @@ test_that("updateParVals works", { makeLogicalParam("f", default = TRUE)) pc = updateParVals(ps, pa, pb) expect_equal(pc, list(a = 0, c = 3, d = 4, e = 5)) - expect_warning(updateParVals(ps, pa, pb, warn = TRUE), "ParamSetting b=2") + expect_warning(updateParVals(ps, pa, pb, warn = TRUE), "ParamSettings \\(b=2\\)") pb2 = list(a = 0, f = FALSE) pc2 = updateParVals(ps, pa, pb2) @@ -55,5 +55,5 @@ test_that("updateParVals works", { pa = list(a = "a1") pb = list() pc = updateParVals(ps, pa, pb) - expect_equal(pc, pb) + expect_equal(pc, pa) }) From b1e6c6fa4712be5a5103d183556dcc193e224708 Mon Sep 17 00:00:00 2001 From: Jakob Richter Date: Mon, 15 Aug 2016 08:44:56 +0200 Subject: [PATCH 6/8] add another test case --- tests/testthat/test_updateParVals.R | 9 +++++++++ 1 file changed, 9 insertions(+) diff --git a/tests/testthat/test_updateParVals.R b/tests/testthat/test_updateParVals.R index 6c9f8323..0f9962d0 100644 --- a/tests/testthat/test_updateParVals.R +++ b/tests/testthat/test_updateParVals.R @@ -60,4 +60,13 @@ test_that("updateParVals works", { pb = list() pc = updateParVals(ps, pa, pb) expect_equal(pc, pa) + + ps = makeParamSet( + makeIntegerParam("a", default = 10L) + ) + pa = list(a = 0L) + pb = list() + pc = updateParVals(ps, pa, pb) + expect_equal(pc, pa) + }) From 3f3a7c3217be7fd037a3c365a2fa87dc1558cd56 Mon Sep 17 00:00:00 2001 From: Jakob Richter Date: Mon, 15 Aug 2016 10:54:31 +0200 Subject: [PATCH 7/8] algo update --- R/updateParVals.R | 22 ++++++++++++++-------- 1 file changed, 14 insertions(+), 8 deletions(-) diff --git a/R/updateParVals.R b/R/updateParVals.R index 62674504..137d0b72 100644 --- a/R/updateParVals.R +++ b/R/updateParVals.R @@ -22,14 +22,20 @@ updateParVals = function(par.set, old.par.vals, new.par.vals, warn = FALSE) { default.par.vals = getDefaults(par.set) # First we extend both par.vals lists with the defaults to get the fully requirements meeting par.vals lists old.with.defaults = updateParVals2(par.set = par.set, old.par.vals = default.par.vals, new.par.vals = old.par.vals) + updated.old = attr(old.with.defaults, "updated") new.with.defaults = updateParVals2(par.set = par.set, old.par.vals = default.par.vals, new.par.vals = new.par.vals) - updated.par.set = updateParVals2(par.set = par.set, old.par.vals = old.with.defaults, new.par.vals = new.with.defaults) - kept.old = attr(old.with.defaults, "kept") - kept.new = attr(new.with.defaults, "kept") + updated.new = attr(new.with.defaults, "updated") + # updated in old but not present in new + respect.old.updated = setdiff(names(updated.old)[updated.old], names(new.par.vals)) + # new.defaults that can be updated + new.candidates = setdiff(names(new.with.defaults), respect.old.updated) + updated.par.vals = updateParVals2(par.set = par.set, old.par.vals = old.with.defaults, new.par.vals = new.with.defaults[new.candidates]) # Find out which parmam names were kept in both update processes # this indicates that this was a default and we don't need it, as it is still a valid default. - both.kept = intersect(names(kept.new)[kept.new], names(kept.old)[kept.old]) - result = updated.par.set[names(updated.par.set) %nin% both.kept] + both.updated = union(names(updated.new)[updated.new], names(updated.old)[updated.old]) + result = updated.par.vals[names(updated.par.vals) %in% both.updated] + # order as in par.set + # result = result[match(names(result), getParamIds(par.set))] if (warn) { # detect dropped param settings: warningf("ParamSettings (%s) were dropped.", convertToShortString(old.par.vals[names(old.par.vals) %nin% names(result)])) @@ -38,7 +44,7 @@ updateParVals = function(par.set, old.par.vals, new.par.vals, warn = FALSE) { } updateParVals2 = function(par.set, old.par.vals, new.par.vals) { - kept = setNames(logical(length(new.par.vals)), names(new.par.vals)) + updated = setNames(rep(TRUE, length(new.par.vals)), names(new.par.vals)) repeat { # we include parameters of the old.par.vals if they meet the requirements # we repeat because some parameters of old.par.vals might only meet the requirements after we added others. (chained requirements) @@ -48,11 +54,11 @@ updateParVals2 = function(par.set, old.par.vals, new.par.vals) { if (isTRUE(try(requiresOk(par.set$pars[[pn]], c(new.par.vals, old.par.vals[pn])), silent = TRUE))) { # keep old.par.val in new.par.vals as it meets the requirements new.par.vals[pn] = old.par.vals[pn] - kept[pn] = TRUE + updated[pn] = FALSE } } # break if no changes were made if (identical(candidate.par.names, setdiff(names(old.par.vals), names(new.par.vals)))) break } - setAttribute(new.par.vals, "kept", kept) + setAttribute(new.par.vals, "updated", updated) } \ No newline at end of file From bca70146d93e8ebaae25100a0af6e1e1f82af959 Mon Sep 17 00:00:00 2001 From: Jakob Richter Date: Mon, 15 Aug 2016 17:02:43 +0200 Subject: [PATCH 8/8] make algo more understandable --- R/updateParVals.R | 13 +++++++++---- tests/testthat/test_updateParVals.R | 3 +++ 2 files changed, 12 insertions(+), 4 deletions(-) diff --git a/R/updateParVals.R b/R/updateParVals.R index 137d0b72..b6dab6f2 100644 --- a/R/updateParVals.R +++ b/R/updateParVals.R @@ -25,10 +25,15 @@ updateParVals = function(par.set, old.par.vals, new.par.vals, warn = FALSE) { updated.old = attr(old.with.defaults, "updated") new.with.defaults = updateParVals2(par.set = par.set, old.par.vals = default.par.vals, new.par.vals = new.par.vals) updated.new = attr(new.with.defaults, "updated") - # updated in old but not present in new - respect.old.updated = setdiff(names(updated.old)[updated.old], names(new.par.vals)) - # new.defaults that can be updated - new.candidates = setdiff(names(new.with.defaults), respect.old.updated) + # new.candidates are the par.vals we want to use from new.with.defaults. + # These exclude those values where the defaults got updated by the old.par.vals but the update is not present in the new.par.vals but still we want to keep this update. + # + # updated.old | updated.new | use + # T | T | new + # T | F | old + # F | T | new + # F | F | new(default) + new.candidates = names(new.with.defaults) %nin% setdiff(names(updated.old)[updated.old], names(updated.new)[updated.new]) updated.par.vals = updateParVals2(par.set = par.set, old.par.vals = old.with.defaults, new.par.vals = new.with.defaults[new.candidates]) # Find out which parmam names were kept in both update processes # this indicates that this was a default and we don't need it, as it is still a valid default. diff --git a/tests/testthat/test_updateParVals.R b/tests/testthat/test_updateParVals.R index 0f9962d0..041a5088 100644 --- a/tests/testthat/test_updateParVals.R +++ b/tests/testthat/test_updateParVals.R @@ -68,5 +68,8 @@ test_that("updateParVals works", { pb = list() pc = updateParVals(ps, pa, pb) expect_equal(pc, pa) + pb2 = list(a = 5L) + pc2 = updateParVals(ps, pa, pb2) + expect_equal(pc2, pb2) })