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
Enhance GPSampler performance (other than introducing local search) #5279
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #5279 +/- ##
==========================================
+ Coverage 89.56% 89.58% +0.01%
==========================================
Files 209 209
Lines 13103 13144 +41
==========================================
+ Hits 11736 11775 +39
- Misses 1367 1369 +2 ☔ View full report in Codecov by Sentry. |
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.
Thank you for separating the PR, I left some minor comments.
Co-authored-by: Shuhei Watanabe <47781922+nabenabe0928@users.noreply.github.com>
Co-authored-by: Shuhei Watanabe <47781922+nabenabe0928@users.noreply.github.com>
Co-authored-by: Shuhei Watanabe <47781922+nabenabe0928@users.noreply.github.com>
Co-authored-by: Shuhei Watanabe <47781922+nabenabe0928@users.noreply.github.com>
@nabenabe0928 I applied your comments. PTAL. |
Co-authored-by: Shuhei Watanabe <47781922+nabenabe0928@users.noreply.github.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.
Thank you for your work, LGTM!
@@ -164,7 +166,8 @@ def _fit_kernel_params( | |||
np.log(initial_kernel_params.inverse_squared_lengthscales.detach().numpy()), | |||
[ | |||
np.log(initial_kernel_params.kernel_scale.item()), | |||
np.log(initial_kernel_params.noise_var.item() - minimum_noise), | |||
# We add 0.01 * minimum_noise to initial noise_var to avoid instability. | |||
np.log(initial_kernel_params.noise_var.item() - 0.99 * minimum_noise), |
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.
Based on the comment above (i.e., "add 0.01 * minimum_noise to initial noise_var"), I feel this line should be like below.
np.log(initial_kernel_params.noise_var.item() + 0.01 * minimum_noise)
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.
I think this line is trying to transform the variable so that the search space is gonna be
So probably, what @y0z wanted to say was actually this?
transformed_noise = initial_kernel_params.noise_var.item() - minimum_noise
...=np.log(transformed_noise + 0.01 * minimum_noise)
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.
Memo: The intention is below.
(initial_noise_var + 0.01 * minimum_noise) - minimum_noise = initial_var - 0.99 * minimum_noise.
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
We split PR #5274 into two. This PR introduces all improvements other than local search.
Description of the changes
See #5274.