Fix shape error in optimize_acqf_cyclic #1648
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
the
CLA Signed
|
@esantorella has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator. |
Codecov Report
@@ Coverage Diff @@
## main #1648 +/- ##
===========================================
- Coverage 100.00% 99.99% -0.01%
===========================================
Files 166 165 -1
Lines 14352 14292 -60
===========================================
- Hits 14352 14291 -61
- Misses 0 1 +1
📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more |
|
This pull request was exported from Phabricator. Differential Revision: D42875942 |
esantorella
added a commit
to esantorella/botorch
that referenced
this pull request
Jan 31, 2023
Summary: ## Motivation Fixes pytorch#873 In the past, `optimize_acqf` implicitly needed 3d inputs when there are equality constraints or inequality constraints and fixed_features don't provide the trivial solution, even though it worked with 2d inputs (no b-batches) in other cases. `optimize_acqf_cyclic` passed it 2d inputs, which would not generally work. I initially considered changing `optimize_acqf_cyclic` to pass 3d inputs, but since I found another place where 2d inputs were used, I decided to change `optimize_acqf` so it works with 2d inputs instead. This was not caught because the only usage of `optimize_acqf_cyclic` was in a test that mocked `optimize_acqf`, so `optimize_acqf_cyclic` was never actually run end-to-end. I changed the test for `optimize_acqf_cyclic` to be more end-to-end, at the cost of worse testing of some intermediate properties. We could keep both versions though. [x] Better docstring documentation on input shapes [x] Add a singleton leading b-dimension where initial conditions are 2d Pull Request resolved: pytorch#1648 Test Plan: [x] More end-to-end test of `optimize_acqf_cyclic` that doesn't stub in `optimize_acqf` (see above) [x] more input validation and unit tests for input validation [x] Ran cases that now raise errors without the new error handling, to make sure they were erroring before Differential Revision: D42875942 Pulled By: esantorella fbshipit-source-id: f19c01ca7ed8fa759b63000189ffa9974248dadb
esantorella
force-pushed
the
fix_optimize_acqf_cyclic
branch
from
4e6ccbb to
72e8a83
Compare
esantorella
added a commit
to esantorella/botorch
that referenced
this pull request
Jan 31, 2023
Summary: ## Motivation Fixes pytorch#873 In the past, `optimize_acqf` implicitly needed 3d inputs when there are equality constraints or inequality constraints and fixed_features don't provide the trivial solution, even though it worked with 2d inputs (no b-batches) in other cases. `optimize_acqf_cyclic` passed it 2d inputs, which would not generally work. I initially considered changing `optimize_acqf_cyclic` to pass 3d inputs, but since I found another place where 2d inputs were used, I decided to change `optimize_acqf` so it works with 2d inputs instead. This was not caught because the only usage of `optimize_acqf_cyclic` was in a test that mocked `optimize_acqf`, so `optimize_acqf_cyclic` was never actually run end-to-end. I changed the test for `optimize_acqf_cyclic` to be more end-to-end, at the cost of worse testing of some intermediate properties. We could keep both versions though. [x] Better docstring documentation on input shapes [x] Add a singleton leading b-dimension where initial conditions are 2d Pull Request resolved: pytorch#1648 Test Plan: [x] More end-to-end test of `optimize_acqf_cyclic` that doesn't stub in `optimize_acqf` (see above) [x] more input validation and unit tests for input validation [x] Ran cases that now raise errors without the new error handling, to make sure they were erroring before Differential Revision: D42875942 Pulled By: esantorella fbshipit-source-id: dd17e7aa46b309e02c8cfde359c8a98cb102973f
esantorella
force-pushed
the
fix_optimize_acqf_cyclic
branch
from
72e8a83 to
8146b77
Compare
esantorella
added a commit
to esantorella/botorch
that referenced
this pull request
Jan 31, 2023
Summary: ## Motivation Fixes pytorch#873 In the past, `optimize_acqf` implicitly needed 3d inputs when there are equality constraints or inequality constraints and fixed_features don't provide the trivial solution, even though it worked with 2d inputs (no b-batches) in other cases. `optimize_acqf_cyclic` passed it 2d inputs, which would not generally work. I initially considered changing `optimize_acqf_cyclic` to pass 3d inputs, but since I found another place where 2d inputs were used, I decided to change `optimize_acqf` so it works with 2d inputs instead. This was not caught because the only usage of `optimize_acqf_cyclic` was in a test that mocked `optimize_acqf`, so `optimize_acqf_cyclic` was never actually run end-to-end. I changed the test for `optimize_acqf_cyclic` to be more end-to-end, at the cost of worse testing of some intermediate properties. We could keep both versions though. [x] Better docstring documentation on input shapes [x] Add a singleton leading b-dimension where initial conditions are 2d Pull Request resolved: pytorch#1648 Test Plan: [x] More end-to-end test of `optimize_acqf_cyclic` that doesn't stub in `optimize_acqf` (see above) [x] more input validation and unit tests for input validation [x] Ran cases that now raise errors without the new error handling, to make sure they were erroring before Differential Revision: D42875942 Pulled By: esantorella fbshipit-source-id: 8461b2b4a724a4961cf4781501a55149281dad9b
esantorella
force-pushed
the
fix_optimize_acqf_cyclic
branch
from
8146b77 to
54549e8
Compare
esantorella
added a commit
to esantorella/botorch
that referenced
this pull request
Jan 31, 2023
Summary: ## Motivation Fixes pytorch#873 In the past, `optimize_acqf` implicitly needed 3d inputs when there are equality constraints or inequality constraints and fixed_features don't provide the trivial solution, even though it worked with 2d inputs (no b-batches) in other cases. `optimize_acqf_cyclic` passed it 2d inputs, which would not generally work. I initially considered changing `optimize_acqf_cyclic` to pass 3d inputs, but since I found another place where 2d inputs were used, I decided to change `optimize_acqf` so it works with 2d inputs instead. This was not caught because the only usage of `optimize_acqf_cyclic` was in a test that mocked `optimize_acqf`, so `optimize_acqf_cyclic` was never actually run end-to-end. I changed the test for `optimize_acqf_cyclic` to be more end-to-end, at the cost of worse testing of some intermediate properties. We could keep both versions though. [x] Better docstring documentation on input shapes [x] Add a singleton leading b-dimension where initial conditions are 2d Pull Request resolved: pytorch#1648 Test Plan: [x] More end-to-end test of `optimize_acqf_cyclic` that doesn't stub in `optimize_acqf` (see above) [x] more input validation and unit tests for input validation [x] Ran cases that now raise errors without the new error handling, to make sure they were erroring before Differential Revision: D42875942 Pulled By: esantorella fbshipit-source-id: dea041dde53f9db51020167e3d940ab0473a6436
esantorella
force-pushed
the
fix_optimize_acqf_cyclic
branch
from
54549e8 to
a401d2e
Compare
|
This pull request was exported from Phabricator. Differential Revision: D42875942 |
esantorella
added a commit
to esantorella/botorch
that referenced
this pull request
Jan 31, 2023
Summary: ## Motivation Fixes pytorch#873 In the past, `optimize_acqf` implicitly needed 3d inputs when there are equality constraints or inequality constraints and fixed_features don't provide the trivial solution, even though it worked with 2d inputs (no b-batches) in other cases. `optimize_acqf_cyclic` passed it 2d inputs, which would not generally work. I initially considered changing `optimize_acqf_cyclic` to pass 3d inputs, but since I found another place where 2d inputs were used, I decided to change `optimize_acqf` so it works with 2d inputs instead. This was not caught because the only usage of `optimize_acqf_cyclic` was in a test that mocked `optimize_acqf`, so `optimize_acqf_cyclic` was never actually run end-to-end. I changed the test for `optimize_acqf_cyclic` to be more end-to-end, at the cost of worse testing of some intermediate properties. We could keep both versions though. [x] Better docstring documentation on input shapes [x] Add a singleton leading b-dimension where initial conditions are 2d Pull Request resolved: pytorch#1648 Test Plan: [x] More end-to-end test of `optimize_acqf_cyclic` that doesn't stub in `optimize_acqf` (see above) [x] more input validation and unit tests for input validation [x] Ran cases that now raise errors without the new error handling, to make sure they were erroring before Differential Revision: D42875942 Pulled By: esantorella fbshipit-source-id: a9fd5afa049e912a94d0b3bae36e33e5deddc32a
esantorella
force-pushed
the
fix_optimize_acqf_cyclic
branch
from
a401d2e to
5510748
Compare
|
This pull request was exported from Phabricator. Differential Revision: D42875942 |
1 similar comment
|
This pull request was exported from Phabricator. Differential Revision: D42875942 |
esantorella
added a commit
to esantorella/botorch
that referenced
this pull request
Feb 3, 2023
Summary: ## Motivation Fixes pytorch#873 In the past, `optimize_acqf` implicitly needed 3d inputs when there are equality constraints or inequality constraints and fixed_features don't provide the trivial solution, even though it worked with 2d inputs (no b-batches) in other cases. `optimize_acqf_cyclic` passed it 2d inputs, which would not generally work. I initially considered changing `optimize_acqf_cyclic` to pass 3d inputs, but since I found another place where 2d inputs were used, I decided to change `optimize_acqf` so it works with 2d inputs instead. This was not caught because the only usage of `optimize_acqf_cyclic` was in a test that mocked `optimize_acqf`, so `optimize_acqf_cyclic` was never actually run end-to-end. I changed the test for `optimize_acqf_cyclic` to be more end-to-end, at the cost of worse testing of some intermediate properties. We could keep both versions though. [x] Better docstring documentation on input shapes [x] Add a singleton leading b-dimension where initial conditions are 2d Pull Request resolved: pytorch#1648 Test Plan: [x] More end-to-end test of `optimize_acqf_cyclic` that doesn't stub in `optimize_acqf` (see above) [x] more input validation and unit tests for input validation [x] Ran cases that now raise errors without the new error handling, to make sure they were erroring before [x] Make `_make_linear_constraints` work with 2d inputs so that `optimize_acqf` also does (previously, optimize_acqf only worked in some cases) Reviewed By: Balandat Differential Revision: D42875942 Pulled By: esantorella fbshipit-source-id: b68eb33ed36b5d1ec3c2d5264283ecb128fe6545
esantorella
force-pushed
the
fix_optimize_acqf_cyclic
branch
from
5510748 to
ce3b9ee
Compare
esantorella
added a commit
to esantorella/botorch
that referenced
this pull request
Feb 3, 2023
Summary: ## Motivation Fixes pytorch#873 In the past, `optimize_acqf` implicitly needed 3d inputs when there are equality constraints or inequality constraints and fixed_features don't provide the trivial solution, even though it worked with 2d inputs (no b-batches) in other cases. `optimize_acqf_cyclic` passed it 2d inputs, which would not generally work. I initially considered changing `optimize_acqf_cyclic` to pass 3d inputs, but since I found another place where 2d inputs were used, I decided to change `optimize_acqf` so it works with 2d inputs instead. This was not caught because the only usage of `optimize_acqf_cyclic` was in a test that mocked `optimize_acqf`, so `optimize_acqf_cyclic` was never actually run end-to-end. I changed the test for `optimize_acqf_cyclic` to be more end-to-end, at the cost of worse testing of some intermediate properties. We could keep both versions though. [x] Better docstring documentation on input shapes [x] Add a singleton leading b-dimension where initial conditions are 2d Pull Request resolved: pytorch#1648 Test Plan: [x] More end-to-end test of `optimize_acqf_cyclic` that doesn't stub in `optimize_acqf` (see above) [x] more input validation and unit tests for input validation [x] Ran cases that now raise errors without the new error handling, to make sure they were erroring before [x] Make `_make_linear_constraints` work with 2d inputs so that `optimize_acqf` also does (previously, optimize_acqf only worked in some cases) Reviewed By: Balandat Differential Revision: D42875942 Pulled By: esantorella fbshipit-source-id: 7d2519cf7ecd509c377e09e2c1adfb3efbb84e3e
esantorella
force-pushed
the
fix_optimize_acqf_cyclic
branch
from
ce3b9ee to
3bf6cab
Compare
|
This pull request was exported from Phabricator. Differential Revision: D42875942 |
esantorella
added a commit
to esantorella/botorch
that referenced
this pull request
Feb 3, 2023
Summary: ## Motivation Fixes pytorch#873 In the past, `optimize_acqf` implicitly needed 3d inputs when there are equality constraints or inequality constraints and fixed_features don't provide the trivial solution, even though it worked with 2d inputs (no b-batches) in other cases. `optimize_acqf_cyclic` passed it 2d inputs, which would not generally work. I initially considered changing `optimize_acqf_cyclic` to pass 3d inputs, but since I found another place where 2d inputs were used, I decided to change `optimize_acqf` so it works with 2d inputs instead. This was not caught because the only usage of `optimize_acqf_cyclic` was in a test that mocked `optimize_acqf`, so `optimize_acqf_cyclic` was never actually run end-to-end. I changed the test for `optimize_acqf_cyclic` to be more end-to-end, at the cost of worse testing of some intermediate properties. We could keep both versions though. [x] Better docstring documentation on input shapes [x] Add a singleton leading b-dimension where initial conditions are 2d Pull Request resolved: pytorch#1648 Test Plan: [x] More end-to-end test of `optimize_acqf_cyclic` that doesn't stub in `optimize_acqf` (see above) [x] more input validation and unit tests for input validation [x] Ran cases that now raise errors without the new error handling, to make sure they were erroring before [x] Make `_make_linear_constraints` work with 2d inputs so that `optimize_acqf` also does (previously, optimize_acqf only worked in some cases) Reviewed By: Balandat Differential Revision: D42875942 Pulled By: esantorella fbshipit-source-id: a4faed4245d2638fd02189d2657ab42a03127bb1
esantorella
force-pushed
the
fix_optimize_acqf_cyclic
branch
from
3bf6cab to
ca67465
Compare
|
This pull request was exported from Phabricator. Differential Revision: D42875942 |
Summary: ## Motivation Fixes pytorch#873 In the past, `optimize_acqf` implicitly needed 3d inputs when there are equality constraints or inequality constraints and fixed_features don't provide the trivial solution, even though it worked with 2d inputs (no b-batches) in other cases. `optimize_acqf_cyclic` passed it 2d inputs, which would not generally work. I initially considered changing `optimize_acqf_cyclic` to pass 3d inputs, but since I found another place where 2d inputs were used, I decided to change `optimize_acqf` so it works with 2d inputs instead. This was not caught because the only usage of `optimize_acqf_cyclic` was in a test that mocked `optimize_acqf`, so `optimize_acqf_cyclic` was never actually run end-to-end. I changed the test for `optimize_acqf_cyclic` to be more end-to-end, at the cost of worse testing of some intermediate properties. We could keep both versions though. [x] Better docstring documentation on input shapes [x] Add a singleton leading b-dimension where initial conditions are 2d Pull Request resolved: pytorch#1648 Test Plan: [x] More end-to-end test of `optimize_acqf_cyclic` that doesn't stub in `optimize_acqf` (see above) [x] more input validation and unit tests for input validation [x] Ran cases that now raise errors without the new error handling, to make sure they were erroring before [x] Make `_make_linear_constraints` work with 2d inputs so that `optimize_acqf` also does (previously, optimize_acqf only worked in some cases) Reviewed By: Balandat Differential Revision: D42875942 Pulled By: esantorella fbshipit-source-id: 15f5ad47e682d24c7a7ff5efff5b9a9727839b45
esantorella
force-pushed
the
fix_optimize_acqf_cyclic
branch
from
ca67465 to
6680f89
Compare
|
This pull request was exported from Phabricator. Differential Revision: D42875942 |
esantorella
added a commit
to esantorella/botorch
that referenced
this pull request
Feb 8, 2023
Summary: ## Motivation Fixes pytorch#873 In the past, `optimize_acqf` implicitly needed 3d inputs when there are equality constraints or inequality constraints and fixed_features don't provide the trivial solution, even though it worked with 2d inputs (no b-batches) in other cases. `optimize_acqf_cyclic` passed it 2d inputs, which would not generally work. I initially considered changing `optimize_acqf_cyclic` to pass 3d inputs, but since I found another place where 2d inputs were used, I decided to change `optimize_acqf` so it works with 2d inputs instead. This was not caught because the only usage of `optimize_acqf_cyclic` was in a test that mocked `optimize_acqf`, so `optimize_acqf_cyclic` was never actually run end-to-end. I changed the test for `optimize_acqf_cyclic` to be more end-to-end, at the cost of worse testing of some intermediate properties. We could keep both versions though. [x] Better docstring documentation on input shapes [x] Add a singleton leading b-dimension where initial conditions are 2d Pull Request resolved: pytorch#1648 Test Plan: [x] More end-to-end test of `optimize_acqf_cyclic` that doesn't stub in `optimize_acqf` (see above) [x] more input validation and unit tests for input validation [x] Ran cases that now raise errors without the new error handling, to make sure they were erroring before [x] Make `_make_linear_constraints` work with 2d inputs so that `optimize_acqf` also does (previously, optimize_acqf only worked in some cases) Differential Revision: https://internalfb.com/D42875942 Pulled By: esantorella fbshipit-source-id: 274a27da954847fbd278a76d20bf42d5be6b4872
esantorella
added a commit
to esantorella/botorch
that referenced
this pull request
Feb 8, 2023
Summary: ## Motivation Fixes pytorch#873 In the past, `optimize_acqf` implicitly needed 3d inputs when there are equality constraints or inequality constraints and fixed_features don't provide the trivial solution, even though it worked with 2d inputs (no b-batches) in other cases. `optimize_acqf_cyclic` passed it 2d inputs, which would not generally work. I initially considered changing `optimize_acqf_cyclic` to pass 3d inputs, but since I found another place where 2d inputs were used, I decided to change `optimize_acqf` so it works with 2d inputs instead. This was not caught because the only usage of `optimize_acqf_cyclic` was in a test that mocked `optimize_acqf`, so `optimize_acqf_cyclic` was never actually run end-to-end. I changed the test for `optimize_acqf_cyclic` to be more end-to-end, at the cost of worse testing of some intermediate properties. We could keep both versions though. [x] Better docstring documentation on input shapes [x] Add a singleton leading b-dimension where initial conditions are 2d Pull Request resolved: pytorch#1648 Test Plan: [x] More end-to-end test of `optimize_acqf_cyclic` that doesn't stub in `optimize_acqf` (see above) [x] more input validation and unit tests for input validation [x] Ran cases that now raise errors without the new error handling, to make sure they were erroring before [x] Make `_make_linear_constraints` work with 2d inputs so that `optimize_acqf` also does (previously, optimize_acqf only worked in some cases) Differential Revision: https://internalfb.com/D42875942 Pulled By: esantorella fbshipit-source-id: 5eab158fda510241b9c4df82d7f210d4777a6e1a
esantorella
added a commit
to esantorella/botorch
that referenced
this pull request
Feb 8, 2023
Summary: ## Motivation Fixes pytorch#873 In the past, `optimize_acqf` implicitly needed 3d inputs when there are equality constraints or inequality constraints and fixed_features don't provide the trivial solution, even though it worked with 2d inputs (no b-batches) in other cases. `optimize_acqf_cyclic` passed it 2d inputs, which would not generally work. I initially considered changing `optimize_acqf_cyclic` to pass 3d inputs, but since I found another place where 2d inputs were used, I decided to change `optimize_acqf` so it works with 2d inputs instead. This was not caught because the only usage of `optimize_acqf_cyclic` was in a test that mocked `optimize_acqf`, so `optimize_acqf_cyclic` was never actually run end-to-end. I changed the test for `optimize_acqf_cyclic` to be more end-to-end, at the cost of worse testing of some intermediate properties. We could keep both versions though. [x] Better docstring documentation on input shapes [x] Add a singleton leading b-dimension where initial conditions are 2d Pull Request resolved: pytorch#1648 Test Plan: [x] More end-to-end test of `optimize_acqf_cyclic` that doesn't stub in `optimize_acqf` (see above) [x] more input validation and unit tests for input validation [x] Ran cases that now raise errors without the new error handling, to make sure they were erroring before [x] Make `_make_linear_constraints` work with 2d inputs so that `optimize_acqf` also does (previously, optimize_acqf only worked in some cases) Differential Revision: https://internalfb.com/D42875942 Pulled By: esantorella fbshipit-source-id: 6d5b69a23810578e0fb5bd5c9814b774c3e1c45d
|
@esantorella merged this pull request in 2f2b7e2. |
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.
Motivation
Fixes #873
optimize_acqfexpects 3d inputs, butoptimize_acqf_cyclicpasses it 2d inputs.Test Plan
This was not caught because the only usage of
optimize_acqf_cyclicwas in a test that mockedoptimize_acqf, sooptimize_acqf_cyclicwas never actually run end-to-end. I changed the test foroptimize_acqf_cyclicto be more end-to-end, at the cost of worse testing of some intermediate properties. We could keep both versions though.