Some clean-up for MES-based acqusition functions #2769
Closed
Conversation
This file contains hidden or 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: D71051750 |
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #2769 +/- ##
==========================================
- Coverage 99.99% 99.99% -0.01%
==========================================
Files 202 202
Lines 18506 18495 -11
==========================================
- Hits 18505 18494 -11
Misses 1 1 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
saitcakmak
added a commit
to saitcakmak/botorch
that referenced
this pull request
Mar 13, 2025
Summary: - Merges `DiscreteMaxValueBase` into `MaxValueBase`. An abstract base class that only has one subclass, which is also an abstract base class, doesn't really provide much value and creates unreachable code that can only be tested by defining dummy subclasses. - Adds an explicit error when multi-output models are used. Both batched `SingleTaskGP` and `ModelListGP` would error out with different reasons. I suspect the underlying code supports it but e2e support needs some modifications. - Errors out if `expand` is provided to `qMultiFidelityLowerBoundMaxValueEntropy`. It produces outputs of wrong shape, which points to some missing handling of the different tensor shapes within the underlying code. - Adds a very basic test for `expand` with `qMultiFidelityMaxValueEntropy`, which produces output of correct shape. - Updates tests to use actual models rather than mock models. Testing that some functionality (like multi-output model support) works with a mock model doesn't actually mean it works. We should be using mock models a lot more sparingly in tests. Differential Revision: D71051750
saitcakmak
force-pushed
the
export-D71051750
branch
from
ecb6970 to
4f3fcce
Compare
|
This pull request was exported from Phabricator. Differential Revision: D71051750 |
Summary: - Merges `DiscreteMaxValueBase` into `MaxValueBase`. An abstract base class that only has one subclass, which is also an abstract base class, doesn't really provide much value and creates unreachable code that can only be tested by defining dummy subclasses. - Adds an explicit error when multi-output models are used. Both batched `SingleTaskGP` and `ModelListGP` would error out with different reasons. I suspect the underlying code supports it but e2e support needs some modifications. - Errors out if `expand` is provided to `qMultiFidelityLowerBoundMaxValueEntropy`. It produces outputs of wrong shape, which points to some missing handling of the different tensor shapes within the underlying code. - Adds a very basic test for `expand` with `qMultiFidelityMaxValueEntropy`, which produces output of correct shape. - Updates tests to use actual models rather than mock models. Testing that some functionality (like multi-output model support) works with a mock model doesn't actually mean it works. We should be using mock models a lot more sparingly in tests. Reviewed By: esantorella Differential Revision: D71051750
saitcakmak
force-pushed
the
export-D71051750
branch
from
4f3fcce to
c773094
Compare
|
This pull request was exported from Phabricator. Differential Revision: D71051750 |
|
This pull request has been merged in 3dc2838. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
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:
DiscreteMaxValueBaseintoMaxValueBase. An abstract base class that only has one subclass, which is also an abstract base class, doesn't really provide much value and creates unreachable code that can only be tested by defining dummy subclasses.SingleTaskGPandModelListGPwould error out with different reasons. I suspect the underlying code supports it but e2e support needs some modifications.expandis provided toqMultiFidelityLowerBoundMaxValueEntropy. It produces outputs of wrong shape, which points to some missing handling of the different tensor shapes within the underlying code.expandwithqMultiFidelityMaxValueEntropy, which produces output of correct shape.Differential Revision: D71051750