Refactor random strategy #347
Conversation
| @@ -2,6 +2,7 @@ | |||
|
|
|||
| from bofire.data_models.strategies.doe import DoEStrategy | |||
| from bofire.data_models.strategies.factorial import FactorialStrategy | |||
| from bofire.data_models.strategies.polytope import PolytopeSampler | |||
There was a problem hiding this comment.
Since there are now also strategies that are called Sampler the naming becomes somewhat inconsistent, no? RandomStrategy is also just a sampling.
There was a problem hiding this comment.
Fully agree, but I do not have a proper solution, how would you do the naming?
There was a problem hiding this comment.
Hmm... The easiest would be that all are strategies. Which they are. With samplers we probably mean not-predictive, right? Then we would call DoE a sampler and Random also a sampler. Depends on what sampler actually means.
There was a problem hiding this comment.
Yep, it means non-predictive. And something like PolytopeStrategy sounds weird ..., I think I will merge this PR first and we create an issue to solve this naming problem, ok? Or perhaps just schedule a meeting ...
This PR refactors the random strategy: instead of using either polytope sampling or rejection sampling, it uses now a combination of both which allows for the handling of more constraint types by the random strategy.
Docstrings will be also updated.