Remove extraneous current_value computation from input constructor for construct_inputs_qKG
#2520
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
facebook-github-bot
added
the
CLA Signed
|
This pull request was exported from Phabricator. Differential Revision: D62415444 |
|
This pull request was exported from Phabricator. Differential Revision: D62415444 |
ltiao
added a commit
to ltiao/botorch
that referenced
this pull request
Sep 10, 2024
…for `construct_inputs_qKG` (pytorch#2520) Summary: Pull Request resolved: pytorch#2520 The input constructor `construct_inputs_qKG` previously computed `current_value` which is extraneous (it is merely an additive constant in the acquisition function). However, `construct_inputs_qMFKG`, which *does* require an explicit `current_value` (in the cost-aware setting), was piggy-backing off `construct_inputs_qKG`, which I suspect is why this persisted. Having said that, `construct_inputs_qKG` computes `current_value` incorrectly for the purposes of `construct_inputs_qMFKG` anyway (as it failed to take fidelity input dimension into consideration), so it wasn't actually useful or beneficial under any circumstances. With D62391106, which calculates `current_value` correctly for `construct_inputs_qMFKG`, we can safely remove this computation altogether from `construct_inputs_qKG` Differential Revision: D62415444
ltiao
force-pushed
the
export-D62415444
branch
from
d4a5ff6
to
b3f43d5
Compare
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #2520 +/- ##
=======================================
Coverage 99.98% 99.98%
=======================================
Files 193 193
Lines 16963 16966 +3
=======================================
+ Hits 16961 16964 +3
Misses 2 2 ☔ View full report in Codecov by Sentry. |
|
This pull request was exported from Phabricator. Differential Revision: D62415444 |
ltiao
added a commit
to ltiao/botorch
that referenced
this pull request
Sep 10, 2024
…for `construct_inputs_qKG` (pytorch#2520) Summary: Pull Request resolved: pytorch#2520 The input constructor `construct_inputs_qKG` previously computed `current_value` which is extraneous (it is merely an additive constant in the acquisition function). However, `construct_inputs_qMFKG`, which *does* require an explicit `current_value` (in the cost-aware setting), was piggy-backing off `construct_inputs_qKG`, which I suspect is why this persisted. Having said that, `construct_inputs_qKG` computes `current_value` incorrectly for the purposes of `construct_inputs_qMFKG` anyway (as it failed to take fidelity input dimension into consideration), so it wasn't actually useful or beneficial under any circumstances. With D62391106, which calculates `current_value` correctly for `construct_inputs_qMFKG`, we can safely remove this computation altogether from `construct_inputs_qKG` Differential Revision: D62415444
ltiao
force-pushed
the
export-D62415444
branch
from
b3f43d5
to
8feb0c5
Compare
|
This pull request was exported from Phabricator. Differential Revision: D62415444 |
ltiao
added a commit
to ltiao/botorch
that referenced
this pull request
Sep 10, 2024
…for `construct_inputs_qKG` (pytorch#2520) Summary: Pull Request resolved: pytorch#2520 The input constructor `construct_inputs_qKG` previously computed `current_value` which is extraneous (it is merely an additive constant in the acquisition function). However, `construct_inputs_qMFKG`, which *does* require an explicit `current_value` (in the cost-aware setting), was piggy-backing off `construct_inputs_qKG`, which I suspect is why this persisted. Having said that, `construct_inputs_qKG` computes `current_value` incorrectly for the purposes of `construct_inputs_qMFKG` anyway (as it failed to take fidelity input dimension into consideration), so it wasn't actually useful or beneficial under any circumstances. With D62391106, which calculates `current_value` correctly for `construct_inputs_qMFKG`, we can safely remove this computation altogether from `construct_inputs_qKG` Reviewed By: Balandat Differential Revision: D62415444
ltiao
force-pushed
the
export-D62415444
branch
from
8feb0c5
to
fdb718e
Compare
…for `construct_inputs_qKG` (pytorch#2520) Summary: Pull Request resolved: pytorch#2520 The input constructor `construct_inputs_qKG` previously computed `current_value` which is extraneous (it is merely an additive constant in the acquisition function). However, `construct_inputs_qMFKG`, which *does* require an explicit `current_value` (in the cost-aware setting), was piggy-backing off `construct_inputs_qKG`, which I suspect is why this persisted. Having said that, `construct_inputs_qKG` computes `current_value` incorrectly for the purposes of `construct_inputs_qMFKG` anyway (as it failed to take fidelity input dimension into consideration), so it wasn't actually useful or beneficial under any circumstances. With D62391106, which calculates `current_value` correctly for `construct_inputs_qMFKG`, we can safely remove this computation altogether from `construct_inputs_qKG` Reviewed By: Balandat Differential Revision: D62415444
|
This pull request was exported from Phabricator. Differential Revision: D62415444 |
ltiao
force-pushed
the
export-D62415444
branch
from
fdb718e
to
8e23e85
Compare
|
This pull request has been merged in ad4a93a. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Summary:
The input constructor
construct_inputs_qKGpreviously computedcurrent_valuewhich is extraneous (it is merely an additive constant in the acquisition function).However,
construct_inputs_qMFKG, which does require an explicitcurrent_value(in the cost-aware setting), was piggy-backing offconstruct_inputs_qKG, which I suspect is why this persisted.Having said that,
construct_inputs_qKGcomputescurrent_valueincorrectly for the purposes ofconstruct_inputs_qMFKGanyway (as it failed to take fidelity input dimension into consideration), so it wasn't actually useful or beneficial under any circumstances.With D62391106, which calculates
current_valuecorrectly forconstruct_inputs_qMFKG, we can safely remove this computation altogether fromconstruct_inputs_qKGDifferential Revision: D62415444