Fix a minor bug in GPSampler for objective that returns inf#5995
Fix a minor bug in GPSampler for objective that returns inf#5995gen740 merged 9 commits intooptuna:masterfrom
inf#5995Conversation
inf
|
I believe the previous logic computed the maximum over all elements in |
Verification Codeimport numpy as np
def original(values: np.ndarray) -> tuple[np.ndarray, np.ndarray]:
finite_vals = values[np.isfinite(values)]
best_finite_val = np.max(finite_vals, axis=0, initial=0.0)
worst_finite_val = np.min(finite_vals, axis=0, initial=0.0)
return best_finite_val, worst_finite_val
def this_pr(values: np.ndarray) -> tuple[np.ndarray, np.ndarray]:
finite_vals_with_nan = np.where(np.isfinite(values), values, np.nan)
is_any_finite = np.any(np.isfinite(finite_vals_with_nan), axis=0)
best_finite_vals = np.where(is_any_finite, np.nanmax(finite_vals_with_nan, axis=0), 0.0)
worst_finite_vals = np.where(is_any_finite, np.nanmin(finite_vals_with_nan, axis=0), 0.0)
return best_finite_vals, worst_finite_vals |
@sawa3030 First of all, we expect the shapes of So we should probably add some unit tests for this part. But again, this routine isn't really executed for most use cases, so negative user impacts are very limited. |
|
@sawa3030 |
|
Thank you for the explanation and for adding the unit tests. LGTM |
c-bata
left a comment
There was a problem hiding this comment.
@nabenabe0928 Changes look almost good to me. I left one comment though.
tests/gp_tests/test_gp.py
Outdated
| @pytest.mark.parametrize( | ||
| "values,ans", | ||
| [ | ||
| (np.array([-1, 0, 1]), np.array([-1, 0, 1])), | ||
| (np.array([-1, -np.inf, 0, np.inf, 1]), np.array([-1, -1, 0, 1, 1])), | ||
| (np.array([[-1, 2], [0, -2], [1, 0]]), np.array([[-1, 2], [0, -2], [1, 0]])), | ||
| ( | ||
| np.array([[-1, 2], [-np.inf, np.inf], [0, -np.inf], [np.inf, -2], [1, 0]]), | ||
| np.array([[-1, 2], [-1, 2], [0, -2], [1, -2], [1, 0]]), | ||
| ), | ||
| ( | ||
| np.array( | ||
| [ | ||
| [-100, np.inf, 10], | ||
| [-np.inf, np.inf, 100], | ||
| [-10, -np.inf, np.inf], | ||
| [np.inf, np.inf, -np.inf], | ||
| ] | ||
| ), | ||
| np.array([[-100, 0, 10], [-100, 0, 100], [-10, 0, 100], [-10, 0, 10]]), | ||
| ), | ||
| (np.array([-np.inf, np.inf]), np.array([0, 0])), | ||
| (np.array([]), np.array([])), | ||
| ], | ||
| ) | ||
| def test_warn_and_convert_inf(values: np.ndarray, ans: np.ndarray) -> None: | ||
| assert np.allclose(warn_and_convert_inf(values), ans) | ||
| if len(values.shape) == 1: | ||
| # Test also with the shape of (n, 1) to ensure the batched version. | ||
| assert np.allclose(warn_and_convert_inf(values[:, np.newaxis]), ans[:, np.newaxis]) |
There was a problem hiding this comment.
This test code would be more readable if it were split into two test cases: one for single-dimensional arrays and another for two-dimensional arrays. What do you think?
There was a problem hiding this comment.
I addressed your comment, PTAL!
Motivation
This PR fixes a minor bug in
GPSampler.The bug of my interest was embedded here:
In principle, this change does not affect most users because this statement is triggered only if a user-defined objective function returns non-finite values.
The bug was originally introduced because of the following misunderstanding:
initialargument innp.maxandnp.mindoes not affect the result as long as the array of interest is not empty.However, this is not actually true.
For example,
np.max([1], initial=10)gives us10instead of1, which we would like to yield in fact.To this end, I address this issue by modifying the corresponding part.
Note that
emmralso needs to be fixed due to the same issue.Description of the changes