search

experimental-design / bofire

Experimental design and (multi-objective) bayesian optimization.
https://experimental-design.github.io/bofire/
BSD 3-Clause "New" or "Revised" License
188 stars 22 forks source link

Refactor Random Strategy II #352

Closed jduerholt closed 6 months ago

jduerholt commented 6 months ago

In this PR the PolytopeSampler Strategy is removed and completely integrated into the RandomStrategy, furthermore UniversalConstraintSampler is renamed into SpaceFillingStrategy as discussed on Friday.

jduerholt commented 6 months ago

@dlinzner-bcs @Osburg test_n_zero_eigvals_constrained is failing for fully-quadratic and I have no idea why. The sampling preserves the equality constraint and still it is failing but only for fully-quadratic. Any idea why?

jduerholt commented 6 months ago

I investigated this a bit more, but I have no idea what is going on here :D, the sampling procedure is from what I see exactly the same but in the original case, we have 7 zero eigenvalues and here we have 8. No idea why.

Osburg commented 6 months ago

Hi @jduerholt, I think the problem lies in the sampling itself. In RandomStrategy._sample_from_polytope() we call sample() for each categorical feature with the same seed.

        for feat in domain.get_features([CategoricalInput, DiscreteInput]):
            samples[feat.key] = feat.sample(n, seed=seed)  # type: ignore

I think we should not do this since it creates regular patterns in the drawn values between the features. In order to fix the test, incrementing the seed by one in each loop iteration does the trick for me (and should still lead to reproducable behavior). 

        for feat in domain.get_features([CategoricalInput, DiscreteInput]):
            seed += 1
            samples[feat.key] = feat.sample(n, seed=seed)  # type: ignore

I guess similar (possibly undetected) issues may occur in other places where sampling methods are called multiple times with the same seed.

jduerholt commented 6 months ago

@Osburg, thank you very much for the catch! I overlooked it when refactoring the random strategy!

jduerholt commented 6 months ago

@bertiqwerty: it is fixed now and ready for review!