[Bug] sample_polytope step size is correlated with direction when number of samples is small #2156
Labels
Comments
|
Hi @sbernasek-albert. Apologies for the late response. The proposed fix seems quite reasonable. We'd be happy to accept a PR implementing the fix. |
zankin
added a commit
to zankin/botorch
that referenced
this issue
Apr 9, 2024
facebook-github-bot
pushed a commit
that referenced
this issue
May 10, 2024
… (#2290) Summary: <!-- Thank you for sending the PR! We appreciate you spending the time to make BoTorch better. Help us understand your motivation by explaining why you decided to make this change. You can learn more about contributing to BoTorch here: https://github.com/pytorch/botorch/blob/main/CONTRIBUTING.md --> ## Motivation This is a quick fix of #2156. In the ```botorch.utils.sampling.sample_polytope()```, both the step size ```rands``` and direction ```Rs``` are currently sampled using the same seed value. This can lead to strong correlation and induce sampling bias, particularly with a small number of steps (e.g., ```n=1```), as illustrated in #2156. Adding ```+1``` to the seed value for ```Rs``` helps to avoid such behaviour. ### Have you read the [Contributing Guidelines on pull requests](https://github.com/pytorch/botorch/blob/main/CONTRIBUTING.md#pull-requests)? Yes. Pull Request resolved: #2290 Test Plan: Not sure if any unit tests needed for that change. Reviewed By: esantorella Differential Revision: D57208858 Pulled By: Balandat fbshipit-source-id: 0653e751f4351a8f9ad2cf2625c93a3a5de7de63
|
Fixed by #2290 |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
🐛 Bug
In the
botorch.utils.sampling.sample_polytope()function the same seed is used to generate both the random step sizes and the random step directions. For very small numbers of steps (e.g. n=1), this causes the the step size and step direction to be strongly correlated, yielding strong sampling bias.This is irrelevant for those using the
optimize_acqfprocedure that leverages large batches of hit-and-run samples generated in parallel. However, for custom optimization routines that require sequential hit-and-run samples, it may be necessary to callsample_polytope(..., n=1).Looping through many such sequential calls yields this correlation between step size (
rands) and step direction (Rs):A super simple fix would be to add 1 to the seed used to generate a step direction, e.g.
sample_hypersphere(..., seed=seed+1). This eliminates any correlation between step-size and step-direction.The text was updated successfully, but these errors were encountered: