Fix #201: focussearch now handles discrete vector parameters (cont.) #257

jakob-r · 2016-09-19T14:02:09Z

I was not able to take over PR #202 and keep it there. But I was able to pull the branch here it is

also general focussearch bugfixes

Parameters that have discrete vector params in their requirement can not be handled by infillOptFocus, so trying to do that now gives an error.

mllg · 2016-11-18T11:10:59Z

R/infillOptFocus.R

+               to.del = sample(val.names, 1)
+               newmap = newmap[newmap != to.del]
+               names(newmap) = names(par$values)
+               discrete.vector.mapping[[dfindex]] <<- newmap


we need the updated discrete.vector.mapping outside of the lapply

mllg · 2016-11-18T11:11:17Z

R/infillOptFocus.R

+           if (par$type %nin% c("discretevector", "logicalvector")) {
+             val.names = names(par$values)
+             # remove current val from delete options, should work also for NA
+             val.names = val.names[!sapply(par$values, identical, y=val)]  # remember, 'val' can be any type


mllg · 2016-11-18T11:11:30Z

R/infillOptFocus.R

+           }
+         } else if (isDiscrete(par)) {
+           # randomly drop a level, which is not val
+           if (length(par$values) <= 1L) {


mllg · 2016-11-18T11:12:45Z

tests/testthat/test_infill_opt_focus.R

@@ -68,13 +68,18 @@ test_that("complex param space, dependencies, focusing, restarts", {
      if(x$disc2 == 'a') tmp3 = log(x$realA) + x$intA^4 + ifelse(x$discA == 'm', 5, 0)
      if(x$disc2 == 'b') tmp3 = exp(x$realB) + ifelse(x$discB == 'R', sin(x$realBR), sin(x$realBNR))
      if(x$disc2 == "c") tmp3 = 500
+      assert(is.list(x$discVec))
+      assert(x$discVec[[1]] %in% c("a", "b", "c"))
+      assert(x$discScal %in% c("x", "y", "z"))


This is not what assert is for. Use expect_is/expect_true and expect_subset

berndbischl · 2016-11-18T14:38:54Z

i will take this now.

ja-thomas · 2016-11-24T15:32:10Z

Just had a look,

a) general look/review
b) rebased to master
c) fixed most of the stuff michel mentioned

afaik good2go now (if travis is green)

ja-thomas · 2016-12-19T08:59:48Z

ping @berndbischl

berndbischl · 2016-12-23T16:47:56Z

this is really not so good what happens here:

i cannot reproduce the error with the suggested new unit test. if i run that test, without further code changes, no error is reproduced. so the unit test is not "catching"
the suggested fix looks suboptimal. code becomes cluttered and unelegant. my suggestion is:
use the new splitVectorParamsFunction from PH, convert the parset in the focus-search to only "single" params and work on those. that should be much simpler.

jakob-r · 2017-01-05T11:01:08Z

Okay. So we are waiting for the PR in ParamHelpers for "splitVectorParamsFunction" and then we will refactor the code.

mb706 and others added 5 commits March 6, 2016 00:22

Fix #201: focussearch now handles discrete vector parameters

5b32e6e

also general focussearch bugfixes

More concise handling of discrete vector params in infillOptFocus

d451c63

Parameters that have discrete vector params in their requirement can not be handled by infillOptFocus, so trying to do that now gives an error.

Cleaning up code.

ecdeca8

Merge branch 'master' into fix201_focussearch

6befed2

naming according to styleguide

15b7e6c

jakob-r mentioned this pull request Sep 19, 2016

Fix #201: focussearch now handles discrete vector parameters #202

Closed

jakobbossek assigned jakobbossek and berndbischl and unassigned jakobbossek Oct 26, 2016

jakobbossek added this to the v0.1 milestone Oct 26, 2016

jakobbossek mentioned this pull request Oct 27, 2016

FocusSearch doesn't handle discrete vector parameters #201

Open

jakob-r added prio-high ready for merge (?) labels Nov 17, 2016

mllg reviewed Nov 18, 2016

View reviewed changes

R/infillOptFocus.R

}

} else if (isDiscrete(par)) {

# randomly drop a level, which is not val

if (length(par$values) <= 1L) {

Copy link

Member

mllg Nov 18, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

== 0

mllg reviewed Nov 18, 2016

View reviewed changes

berndbischl force-pushed the fix201_focussearch branch from 15b7e6c to 9108ff2 Compare November 18, 2016 20:59

ja-thomas force-pushed the fix201_focussearch branch from 9108ff2 to d167503 Compare November 24, 2016 15:28

ja-thomas force-pushed the fix201_focussearch branch from 0febb72 to a1eb197 Compare November 30, 2016 11:53

berndbischl force-pushed the fix201_focussearch branch from c3cfc07 to 9648acf Compare December 22, 2016 14:19

jakob-r mentioned this pull request May 12, 2017

new fun splitVecParams NOT DONE mlr-org/ParamHelpers#151

Open

jakob-r force-pushed the fix201_focussearch branch from 9648acf to 15b7e6c Compare May 22, 2017 08:58

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix #201: focussearch now handles discrete vector parameters (cont.) #257

Fix #201: focussearch now handles discrete vector parameters (cont.) #257

jakob-r commented Sep 19, 2016

mllg Nov 18, 2016

ja-thomas Nov 24, 2016

mllg Nov 18, 2016

mllg Nov 18, 2016

mllg Nov 18, 2016

berndbischl commented Nov 18, 2016

ja-thomas commented Nov 24, 2016

ja-thomas commented Dec 19, 2016

berndbischl commented Dec 23, 2016

jakob-r commented Jan 5, 2017 •

edited

Loading

Fix #201: focussearch now handles discrete vector parameters (cont.) #257

Are you sure you want to change the base?

Fix #201: focussearch now handles discrete vector parameters (cont.) #257

Conversation

jakob-r commented Sep 19, 2016

mllg Nov 18, 2016

Choose a reason for hiding this comment

ja-thomas Nov 24, 2016

Choose a reason for hiding this comment

mllg Nov 18, 2016

Choose a reason for hiding this comment

mllg Nov 18, 2016

Choose a reason for hiding this comment

mllg Nov 18, 2016

Choose a reason for hiding this comment

berndbischl commented Nov 18, 2016

ja-thomas commented Nov 24, 2016

ja-thomas commented Dec 19, 2016

berndbischl commented Dec 23, 2016

jakob-r commented Jan 5, 2017 • edited Loading

jakob-r commented Jan 5, 2017 •

edited

Loading