Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Open
wants to merge 5 commits into
base: main
Choose a base branch
from

Conversation

jakob-r
Copy link
Member

@jakob-r jakob-r commented Sep 19, 2016

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

mb706 and others added 5 commits March 6, 2016 00:22
Parameters that have discrete vector params in their requirement can
not be handled by infillOptFocus, so trying to do that now gives an
error.
to.del = sample(val.names, 1)
newmap = newmap[newmap != to.del]
names(newmap) = names(par$values)
discrete.vector.mapping[[dfindex]] <<- newmap
Copy link
Member

Choose a reason for hiding this comment

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

<<-?

Copy link
Contributor

Choose a reason for hiding this comment

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

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

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
Copy link
Member

Choose a reason for hiding this comment

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

vlapply

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

Choose a reason for hiding this comment

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

== 0

@@ -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"))
Copy link
Member

Choose a reason for hiding this comment

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

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

@berndbischl
Copy link
Member

i will take this now.

@ja-thomas
Copy link
Contributor

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
Copy link
Contributor

ping @berndbischl

@berndbischl
Copy link
Member

this is really not so good what happens here:

  1. 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"

  2. 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
Copy link
Member Author

jakob-r commented Jan 5, 2017

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants