Remove correlation between the step size and the step direction (#2156) #2290
Conversation
facebook-github-bot
added
the
CLA Signed
| @@ -250,7 +250,7 @@ def sample_polytope( | |||
| d = x0.size(0) | |||
| # uniform samples from unit ball in d dims | |||
| Rs = sample_hypersphere( | |||
| d=d, n=n_tot, dtype=A.dtype, device=A.device, seed=seed | |||
| d=d, n=n_tot, dtype=A.dtype, device=A.device, seed=seed + 1 | |||
There was a problem hiding this comment.
Would you mind adding a brief comment for why this is necessary, that links to #2156 please?
There was a problem hiding this comment.
Done. Hope it is clearer now 🙂
|
Thanks for the PR! It looks good, but it would be great to add a comment on why this needed in line, for posterity. |
|
Sorry for the delay @zankin, sdaulton has been out. Could you add the link to the issue as per my suggestion? Then I'm happy to merge this in. |
Thanks! I've included a link to the issue in the comment line. |
|
@Balandat has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator. |
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #2290 +/- ##
========================================
Coverage 99.97% 99.97%
========================================
Files 197 189 -8
Lines 17165 16669 -496
========================================
- Hits 17160 16665 -495
+ Misses 5 4 -1 ☔ View full report in Codecov by Sentry. |
Motivation
This is a quick fix of #2156.
In the
botorch.utils.sampling.sample_polytope(), both the step sizerandsand directionRsare 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+1to the seed value forRshelps to avoid such behaviour.Have you read the Contributing Guidelines on pull requests?
Yes.
Test Plan
Not sure if any unit tests needed for that change.