-
-
Notifications
You must be signed in to change notification settings - Fork 971
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
Support batched sampling with BoTorch #4591
Support batched sampling with BoTorch #4591
Conversation
Changing the signature of I personally think that it is OK because 1. But it needs to be discussed. It is also possible to avoid breaking change by adding a try/catch or a condition on calling |
Thank you for the preliminary review! I added a test so the PR is ready for review. |
Codecov Report
📣 This organization is not using Codecov’s GitHub App Integration. We recommend you install it so Codecov can continue to function properly for your repositories. Learn more @@ Coverage Diff @@
## master #4591 +/- ##
==========================================
+ Coverage 90.88% 90.89% +0.01%
==========================================
Files 184 184
Lines 13959 13983 +24
==========================================
+ Hits 12686 12710 +24
Misses 1273 1273
📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more |
Following the comment by @contramundum53, I added a commit that would ensure backward compatibility. As discussed above, it seems to be a bit of overengineering and makes If you are happy with the current implementation, I will add an additional test for checking the backward compatibility and warning. |
This pull request has not seen any recent activity. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the update. I have several comments. PTAL.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry for my late reply 🙇
I added some comments.
Thank you for the review! I should have addressed all of your reviews. PTAL |
Co-authored-by: contramundum53 <contramundum53@outlook.com>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM! Thanks for the PR!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM.
Motivation
Currently, botorch integration of Optuna does not take the running trials into account when calculating the acquisition function. (By the way, in contrast, TPE has the
constant_lier
option that tries to leverage the information of running trials.) However, the original botorch API can leverage this information, and that seems to substantially improve parallel optimization performance.On a simple benchmark using 10-dimensional quadratic function (y = sum_{i=0, ..., 9} |x_i|^2 with -5 <= x_i <= 5), with n_trials = 40 and n_workers=10, the average objective values were 4.55 ± 0.22 (w/ batching) vs 10.30 ± 0.59 (w/o batching). The script for the benchmark can be found at https://gist.github.com/kstoneriv3/1e877830e535d9ebea0f9d55ed50734c.
Description of the changes
I just need to change several lines of code at
optuna.integration.botorch
for passing the information about running trials to the BoTorch API.The test is WIP.