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

Refactor Random Strategy II #352

Merged
merged 4 commits into from
Feb 13, 2024
Merged

Refactor Random Strategy II #352

merged 4 commits into from
Feb 13, 2024

Conversation

jduerholt
Copy link
Contributor

@jduerholt jduerholt commented Feb 10, 2024

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

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

jduerholt commented Feb 12, 2024

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

Osburg commented Feb 12, 2024

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

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

@jduerholt
Copy link
Contributor Author

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

Copy link
Contributor

@bertiqwerty bertiqwerty left a comment

Choose a reason for hiding this comment

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

Thanks, Johannes.

@jduerholt jduerholt merged commit 7d6af93 into main Feb 13, 2024
10 checks passed
@jduerholt jduerholt deleted the refactor/random_strategy2 branch February 13, 2024 15:50
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