Add unit tests for batched L-BFGS-B#6274
Conversation
There was a problem hiding this comment.
Could you make this PR only for this test file?
f7c6520 to
da11c03
Compare
tests/samplers_tests/test_gp.py
Outdated
| import optuna._gp.batched_lbfgsb as my_module | ||
|
|
||
| importlib.reload(my_module) | ||
| assert my_module._greenlet_imports.is_successful() is False |
There was a problem hiding this comment.
| assert my_module._greenlet_imports.is_successful() is False | |
| assert not my_module._greenlet_imports.is_successful() |
| _verify_results(X0, func_and_grad, kwargs_ours, kwargs_scipy) | ||
|
|
||
|
|
||
| def test_greenlet_import_behavior(monkeypatch: pytest.MonkeyPatch) -> None: |
There was a problem hiding this comment.
Do we need this test? I feel like it's tested in the other tests. It would be better avoid duplications to not doubly manage similar tests.
There was a problem hiding this comment.
Thanks for your comment. As you pointed out, this test is verbose, so I will remove it.
| import optuna._gp.batched_lbfgsb as my_module | ||
|
|
||
| importlib.reload(my_module) | ||
| assert my_module._greenlet_imports.is_successful() is use_greenlet |
There was a problem hiding this comment.
Please use == to check the value validity.
| assert my_module._greenlet_imports.is_successful() is use_greenlet | |
| assert my_module._greenlet_imports.is_successful() == use_greenlet |
| "lower_bound,upper_bound", [(-np.inf, None), (None, np.inf), (-np.inf, np.inf), (None, None)] | ||
| ) | ||
| @pytest.mark.parametrize("dim, n_localopts", [(10, 10), (1, 10), (10, 1), (1, 1)]) | ||
| def test_batched_lbfgsb_without_bounds( |
There was a problem hiding this comment.
Why not using use_greenlet in this function?
nabenabe0928
left a comment
There was a problem hiding this comment.
Mostly LGTM. Please address my comments 🙇
|
Thank you for your review! I addressed the issues you mentioned. |
nabenabe0928
left a comment
There was a problem hiding this comment.
Thank you for the update, LGTM!
| bounds[:, 1] = upper_bound | ||
| kwargs_ours.update(bounds=bounds) | ||
| kwargs_scipy.update(bounds=bounds) | ||
| _verify_results(X0, func_and_grad, kwargs_ours, kwargs_scipy) |
There was a problem hiding this comment.
Is batched_lbfgsb_func=my_module.batched_lbfgsb not required here?
There was a problem hiding this comment.
No, we do not need it.
The fallback automatically happens in batched_l_bfgs_b.
There was a problem hiding this comment.
I modified the implementation to explicitly pass batched_lbfgsb_func to _verify_results.
Motivation
Add unit tests for
batched_lbfgsmodule. This is the followup PR of this PR:GPSamplerby Batching Acquisition Function Evaluations #6268Description of the changes
This change add the unit tests for
batched_lbfgsbwith/withoutgreenletand an end-to-end test wheregreenletis unavailable andbatched_lbfgsbshould be fallen back.