Continuing cleanup of optimize_acqf #1676
Closed
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
CLA Signed
|
This pull request was exported from Phabricator. Differential Revision: D43200823 |
esantorella
added a commit
to esantorella/botorch
that referenced
this pull request
Feb 14, 2023
Summary: Pull Request resolved: pytorch#1676 The usage of `kwargs` in `optimize_acqf` and related functions has a few downsides: - It's not transparent to the user what options are supported - When kwargs get mutated by `.pop()`, this can cause subtle errors - If a user provides an unsupported option, there will be no error or warning. In this diff I change some options in `kwargs` to named arguments. I did not entirely remove `kwargs`, because they serve a few purposes, all of which are kind of iffy. I'll attack that in the next diff in the stack. These purposes are: - Some kwargs, now marked as `ic_gen_kwargs`, get passed from `optimize_acqf` to a function that generates ICs. This will only happen if the user provides a function with a different signature from the BoTorch IC generator functions. I'm not sure anyone was actually using that functionality. Tests still pass if the `kwargs` arguement in `optimize_acqf` is removed. - Users may pass incorrect keyword arguments for no good reason. This fails silently. - Ax `botorch_modular` passes the same signature to a variety of optimizers when the user does not specify otherwise. So it passes keyword arguments that don't do anything, but for a good reason. I think it would make sense to have BoTorch raise a warning and Ax silence it. Differential Revision: D43200823 fbshipit-source-id: 45a0b7e08cfef6e1eb572871549f1eb7fdcc8b24
esantorella
force-pushed
the
export-D43200823
branch
from
6e77863 to
5080529
Compare
|
This pull request was exported from Phabricator. Differential Revision: D43200823 |
Codecov Report
@@ Coverage Diff @@
## main #1676 +/- ##
=========================================
Coverage 100.00% 100.00%
=========================================
Files 169 169
Lines 14523 14522 -1
=========================================
- Hits 14523 14522 -1
📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more |
esantorella
added a commit
to esantorella/botorch
that referenced
this pull request
Feb 14, 2023
Summary: Pull Request resolved: pytorch#1676 The usage of `kwargs` in `optimize_acqf` and related functions has a few downsides: - It's not transparent to the user what options are supported - When kwargs get mutated by `.pop()`, this can cause subtle errors - If a user provides an unsupported option, there will be no error or warning. In this diff I change some options in `kwargs` to named arguments. I did not entirely remove `kwargs`, because they serve a few purposes, all of which are kind of iffy. I'll attack that in the next diff in the stack. These purposes are: - Some kwargs, now marked as `ic_gen_kwargs`, get passed from `optimize_acqf` to a function that generates ICs. This will only happen if the user provides a function with a different signature from the BoTorch IC generator functions. I'm not sure anyone was actually using that functionality. Tests still pass if the `kwargs` arguement in `optimize_acqf` is removed. - Users may pass incorrect keyword arguments for no good reason. This fails silently. - Ax `botorch_modular` passes the same signature to a variety of optimizers when the user does not specify otherwise. So it passes keyword arguments that don't do anything, but for a good reason. I think it would make sense to have BoTorch raise a warning and Ax silence it. Differential Revision: D43200823 fbshipit-source-id: c7679451da10b5934bf3c55e4909d418cfa72a70
esantorella
force-pushed
the
export-D43200823
branch
from
5080529 to
538d443
Compare
|
This pull request was exported from Phabricator. Differential Revision: D43200823 |
esantorella
added a commit
to esantorella/botorch
that referenced
this pull request
Feb 14, 2023
Summary: Pull Request resolved: pytorch#1676 The usage of `kwargs` in `optimize_acqf` and related functions has a few downsides: - It's not transparent to the user what options are supported - When kwargs get mutated by `.pop()`, this can cause subtle errors - If a user provides an unsupported option, there will be no error or warning. In this diff I change some options in `kwargs` to named arguments. I did not entirely remove `kwargs`, because they serve a few purposes, all of which are kind of iffy. I'll attack that in the next diff in the stack. These purposes are: - Some kwargs, now marked as `ic_gen_kwargs`, get passed from `optimize_acqf` to a function that generates ICs. This will only happen if the user provides a function with a different signature from the BoTorch IC generator functions. I'm not sure anyone was actually using that functionality. Tests still pass if the `kwargs` arguement in `optimize_acqf` is removed. - Users may pass incorrect keyword arguments for no good reason. This fails silently. - Ax `botorch_modular` passes the same signature to a variety of optimizers when the user does not specify otherwise. So it passes keyword arguments that don't do anything, but for a good reason. I think it would make sense to have BoTorch raise a warning and Ax silence it. Differential Revision: https://internalfb.com/D43200823 fbshipit-source-id: 88cccb5a7093d707bee36a712ebb915588c1ff29
esantorella
added a commit
to esantorella/botorch
that referenced
this pull request
Feb 14, 2023
Summary: Pull Request resolved: pytorch#1676 The usage of `kwargs` in `optimize_acqf` and related functions has a few downsides: - It's not transparent to the user what options are supported - When kwargs get mutated by `.pop()`, this can cause subtle errors - If a user provides an unsupported option, there will be no error or warning. In this diff I change some options in `kwargs` to named arguments. I did not entirely remove `kwargs`, because they serve a few purposes, all of which are kind of iffy. I'll attack that in the next diff in the stack. These purposes are: - Some kwargs, now marked as `ic_gen_kwargs`, get passed from `optimize_acqf` to a function that generates ICs. This will only happen if the user provides a function with a different signature from the BoTorch IC generator functions. I'm not sure anyone was actually using that functionality. Tests still pass if the `kwargs` arguement in `optimize_acqf` is removed. - Users may pass incorrect keyword arguments for no good reason. This fails silently. - Ax `botorch_modular` passes the same signature to a variety of optimizers when the user does not specify otherwise. So it passes keyword arguments that don't do anything, but for a good reason. I think it would make sense to have BoTorch raise a warning and Ax silence it. Differential Revision: https://internalfb.com/D43200823 fbshipit-source-id: b66ac13dba40ccd9e0b2c4e3ef10475d9b3a8167
Summary: Pull Request resolved: pytorch#1676 The usage of `kwargs` in `optimize_acqf` and related functions has a few downsides: - It's not transparent to the user what options are supported - When kwargs get mutated by `.pop()`, this can cause subtle errors - If a user provides an unsupported option, there will be no error or warning. In this diff I change some options in `kwargs` to named arguments. I did not entirely remove `kwargs`, because they serve a few purposes, all of which are kind of iffy. I'll attack that in the next diff in the stack. These purposes are: - Some kwargs, now marked as `ic_gen_kwargs`, get passed from `optimize_acqf` to a function that generates ICs. This will only happen if the user provides a function with a different signature from the BoTorch IC generator functions. I'm not sure anyone was actually using that functionality. Tests still pass if the `kwargs` arguement in `optimize_acqf` is removed. - Users may pass incorrect keyword arguments for no good reason. This fails silently. - Ax `botorch_modular` passes the same signature to a variety of optimizers when the user does not specify otherwise. So it passes keyword arguments that don't do anything, but for a good reason. I think it would make sense to have BoTorch raise a warning and Ax silence it. Differential Revision: D43200823 fbshipit-source-id: 287b998aaf46d5709723e97b3b7fc6b3eea96e5b
esantorella
force-pushed
the
export-D43200823
branch
from
538d443 to
6902041
Compare
|
This pull request was exported from Phabricator. Differential Revision: D43200823 |
|
This pull request has been merged in 718be0c. |
CompRhys
added a commit
to Radical-AI/botorch
that referenced
this pull request
Oct 21, 2024
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 usage of
kwargsinoptimize_acqfand related functions has a few downsides:.pop(), this can cause subtle errorsIn this diff I change some options in
kwargsto named arguments. I did not entirely removekwargs, because they serve a few purposes, all of which are kind of iffy. I'll attack that in the next diff in the stack. These purposes are:ic_gen_kwargs, get passed fromoptimize_acqfto a function that generates ICs. This will only happen if the user provides a function with a different signature from the BoTorch IC generator functions. I'm not sure anyone was actually using that functionality. Tests still pass if thekwargsarguement inoptimize_acqfis removed.botorch_modularpasses the same signature to a variety of optimizers when the user does not specify otherwise. So it passes keyword arguments that don't do anything, but for a good reason. I think it would make sense to have BoTorch raise a warning and Ax silence it.Differential Revision: D43200823