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

add 'distinct' argument to VectorParams #206

Closed
wants to merge 6 commits into from
Closed

add 'distinct' argument to VectorParams #206

wants to merge 6 commits into from

Conversation

pat-s
Copy link
Member

@pat-s pat-s commented Nov 26, 2018

To enable sampling without replacement.

Currently, sampling with replacement is hardcoded for vector params.

This is needed to sample unique filter methods which are going to be used in ensemble filters.

@jakob-r Is this possible in paradox already?

Edit: The fix obv only works for tuneXXX methods that use sampleValue. MBO does not. So maybe we need to tackle this in multiple repos..

@jakob-r
Copy link
Member

jakob-r commented Nov 29, 2018

I don't know if we want to to really deal with that here. It will introduce more problems:

  • isFeasible has to work with it
  • generateGridDesign hast to work with it
  • generateDesign has to work with it

As we already discussed we can transform a logical vector param to a distinct factor vector. Also is this just a restriction which you could also include in the forbidden argument of the param forbidden = quote(anyDuplicated(id.of.numvec)) should do the trick.

I vote for:

  • close this PR
  • allow trafos for logical vector params

@berndbischl
Copy link
Member

i also dont think we should do this here. but can we please explain the "issue" first?
either in an issue here, or in paradox?

@pat-s
Copy link
Member Author

pat-s commented Nov 29, 2018

but can we please explain the "issue" first?

Quoted from above:

"Currently, sampling with replacement is hardcoded for vector params.
This is needed to sample unique filter methods which are going to be used in ensemble filters."

Addition: If you create a "DistinctVectorParam" with let's say 15 values and set len = 8, 8 instances will be sampled WITH replacement out of those 15 choices.
This is hardcoded atm and in my use case, I need sampling without replacement as I do not want to have multiple instances of the same simple filter in this selection.

Should I open an issue in paradox?

@berndbischl
Copy link
Member

i read the code, i also read your text here, still had some problems to understand what exactly is to be adressed.

Should I open an issue in paradox?

yes, i think it makes sense to continue discussing there. i have some things to mention

@berndbischl
Copy link
Member

let me try to summarize here already:

a) the current PH implementation allows only for uniform sampling. you now kinda change that, just for a special case.

b) IMHO we are looking at a design problem. params should not implement their sampler. they should simply descibe params. the sampler should be a separate service class. then this can be specialized

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

Successfully merging this pull request may close these issues.

3 participants