Minor patch to MVNXPB #1933
Minor patch to MVNXPB #1933
Conversation
|
Hi @j-wilson! Thank you for your pull request. We require contributors to sign our Contributor License Agreement, and yours needs attention. You currently have a record in our system, but the CLA is no longer valid, and will need to be resubmitted. ProcessIn order for us to review and merge your suggested changes, please sign at https://code.facebook.com/cla. If you are contributing on behalf of someone else (eg your employer), the individual CLA may not be sufficient and your employer may need to sign the corporate CLA. Once the CLA is signed, our tooling will perform checks and validations. Afterwards, the pull request will be tagged with If you have received this in error or have any questions, please contact us at cla@meta.com. Thanks! |
facebook-github-bot
added
the
CLA Signed
|
@Balandat has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator. |
botorch/utils/probability/mvnxpb.py
Outdated
| bounds_i = bounds_i - torch.einsum( | ||
| "...i,...i", L[..., i, :i].clone(), y[..., :i].clone() | ||
| ).unsqueeze(-1) |
There was a problem hiding this comment.
Why change to einsum here? The parsing of the formula actually has some overhead - I just tested this, for small dimensions the previous implementation is about 5X faster.
There was a problem hiding this comment.
Ah, good catch. I switched to einsum to avoid allocating additional memory, thinking that it would run at roughly the same speed. Since it's significantly slower, we should probably revert.
Codecov Report
@@ Coverage Diff @@
## main #1933 +/- ##
=======================================
Coverage 99.94% 99.94%
=======================================
Files 176 176
Lines 15467 15467
=======================================
Hits 15459 15459
Misses 8 8
📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more |
|
@Balandat has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator. |
Overview
This PR introduces a minor fix to the MVNXPB algorithm, so that bounds are properly whitened when computing initial plug-in estimators (which can impact the pivot order).
Test Plan
I have run the existing unit tests locally.