Do not count hitting maxiter as optimization failure & update default maxiter #1478
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: D41053812 |
Codecov Report
@@ Coverage Diff @@
## main #1478 +/- ##
=========================================
Coverage 100.00% 100.00%
=========================================
Files 149 149
Lines 12935 12945 +10
=========================================
+ Hits 12935 12945 +10
📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more |
Balandat
reviewed
Nov 6, 2022
test/generation/test_gen.py
Outdated
| @@ -69,7 +69,7 @@ def _setUp(self, double=False, expand=False): | |||
| class TestGenCandidates(TestBaseCandidateGeneration): | |||
| def test_gen_candidates(self, gen_candidates=gen_candidates_scipy, options=None): | |||
| options = options or {} | |||
| options = {**options, "maxiter": 5} | |||
| options["maxiter"] = options.get("maxiter", 5) | |||
There was a problem hiding this comment.
This is problematic since if options is passed as a dict, this method will now modify the options dict in-place. We generally should avoid these kinds of side effects to function arguments (unless we specifically want this to happen).
test/generation/test_gen.py
Outdated
| @@ -227,6 +213,15 @@ def test_gen_candidates_scipy_warns_opt_failure(self): | |||
| ) | |||
| self.assertTrue(expected_warning_raised) | |||
|
|
|||
| def test_gen_candidates_scipy_no_maxiter_warning(self): | |||
| # Check that no warnings are raised on hitting maxiter. | |||
There was a problem hiding this comment.
We should probably log this? Could be at the info or maybe even the warning logging level.
saitcakmak
added a commit
to saitcakmak/botorch
that referenced
this pull request
Nov 7, 2022
… maxiter (pytorch#1478) Summary: Pull Request resolved: pytorch#1478 We currently retry optimization upon hitting maxiter, which typically leads to optimizing the same thing twice. This diff updates the warning handling to avoid this behavior. The default maxiter is also updated to 2_000, which is a steep change from scipy defaults of 15_000 for L-BFGS-B and 100 for SLSQP. This is just a temporary setting to bridge the gap between the two until we come up with better stopping conditions for the optimizers. Differential Revision: D41053812 fbshipit-source-id: 88dff72b6ae06ae11b0e9cf70c715d2ccca7f8fc
saitcakmak
force-pushed
the
export-D41053812
branch
from
b0ac7c7 to
0767a55
Compare
|
This pull request was exported from Phabricator. Differential Revision: D41053812 |
saitcakmak
added a commit
to saitcakmak/botorch
that referenced
this pull request
Nov 9, 2022
… maxiter (pytorch#1478) Summary: Pull Request resolved: pytorch#1478 We currently retry optimization upon hitting maxiter, which typically leads to optimizing the same thing twice. This diff updates the warning handling to avoid this behavior. The default maxiter is also updated to 2_000, which is a steep change from scipy defaults of 15_000 for L-BFGS-B and 100 for SLSQP. This is just a temporary setting to bridge the gap between the two until we come up with better stopping conditions for the optimizers. Differential Revision: https://internalfb.com/D41053812 fbshipit-source-id: 0723841d9affb3f34cca92f127c1fbf100850c41
… maxiter (pytorch#1478) Summary: Pull Request resolved: pytorch#1478 We currently retry optimization upon hitting maxiter, which typically leads to optimizing the same thing twice. This diff updates the warning handling to avoid this behavior. The default maxiter is also updated to 2_000, which is a steep change from scipy defaults of 15_000 for L-BFGS-B and 100 for SLSQP. This is just a temporary setting to bridge the gap between the two until we come up with better stopping conditions for the optimizers. Reviewed By: esantorella Differential Revision: D41053812 fbshipit-source-id: a0f27fb72f3cab6343fb55b6041717706e2445ce
saitcakmak
force-pushed
the
export-D41053812
branch
from
0767a55 to
46e83c0
Compare
|
This pull request was exported from Phabricator. Differential Revision: D41053812 |
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:
We currently retry optimization upon hitting maxiter, which typically leads to optimizing the same thing twice. This diff updates the warning handling to avoid this behavior.
The default maxiter is also updated to 2_000, which is a steep change from scipy defaults of 15_000 for L-BFGS-B and 100 for SLSQP. This is just a temporary setting to bridge the gap between the two until we come up with better stopping conditions for the optimizers.
Differential Revision: D41053812