Consolidate benchmark methods and give them a clearer naming system #1878
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: D49566866 |
Codecov ReportAll modified lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #1878 +/- ##
==========================================
+ Coverage 94.56% 94.58% +0.02%
==========================================
Files 460 458 -2
Lines 43531 43502 -29
==========================================
- Hits 41164 41147 -17
+ Misses 2367 2355 -12
☔ View full report in Codecov by Sentry. |
esantorella
force-pushed
the
export-D49566866
branch
from
149848d to
fe664ee
Compare
|
This pull request was exported from Phabricator. Differential Revision: D49566866 |
…acebook#1878) Summary: * No longer have "default" like "SOBOL+BOTORCH_MODULAR::default"; instead, construct method from components (e.g. "SOBOL+BOTORCH_MODULAR::SingleTaskGP_qLogNoisyExpectedImprovement") and *test* that the benchmarks match defaults. * Remove non-Log EI benchmarks, since these have all been migrated. * Define MBM benchmarks more concisely and programmatically. Defining benchmark methods explicitly in terms of their components, without reference to defaults, has a few benefits: 1) One can't accidentally test the same method multiple times; 2) There is a 1:1 mapping between method name and functionality rather than many:many, making it easier to track the performance of methods, and 3) tests will prevent benchmarks from drifting apart from default behavior (in MBM or CGS) over time. Differential Revision: D49566866
esantorella
force-pushed
the
export-D49566866
branch
from
fe664ee to
fc5a825
Compare
|
This pull request was exported from Phabricator. Differential Revision: D49566866 |
|
This pull request has been merged in 636a701. |
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:
Defining benchmark methods explicitly in terms of their components, without reference to defaults, has a few benefits:
One can't accidentally test the same method multiple times;
There is a 1:1 mapping between method name and functionality rather than many:many, making it easier to track the performance of methods, and
tests will prevent benchmarks from drifting apart from default behavior (in MBM or CGS) over time.
Differential Revision: D49566866