TST suppress convergence warnings in coordinate descent #20144
Conversation
These warnings have been fixed by either increasing the number of iterations of fitting, or by suppressing the warnings with pytest.warns For parametrized tests a check is done so that only the failing tests get the pytest.warns context manager, all other get a dummy context manager that doesn't do anything. There is one instance where the warning does not disappear in test_linear_model_sample_weights_normalize_in_pipeline (second context manager call), but I don't know why ...
|
I just saw that other tests use the following style to suppress warnings: Is there a preference for the one or the other? |
- use ignore_warnings instead of pytest.warns since depending on the used Python system the warning might not be issued - deal with multiple warnings in a single statement - use nullcontext() instead of suppress() since we have moved to Python 3.7 as dependency and CI testing system
| @@ -724,14 +780,18 @@ def test_uniform_targets(): | |||
| for model in models_single_task: | |||
| for y_values in (0, 5): | |||
| y1.fill(y_values) | |||
| assert_array_equal(model.fit(X_train, y1).predict(X_test), y1) | |||
| with ignore_warnings(category=ConvergenceWarning): | |||
There was a problem hiding this comment.
since we ignore the warning all the time, we could decorate the test function as well.
| @@ -111,7 +113,10 @@ def test_lasso_zero(): | |||
| # Check that the lasso can handle zero data without crashing | |||
| X = [[0], [0], [0]] | |||
| y = [0, 0, 0] | |||
| clf = Lasso(alpha=0.1).fit(X, y) | |||
| # _cd_fast.pyx tests for gap < tol, but here we get 0.0 < 0.0 | |||
There was a problem hiding this comment.
Actually I do not understand why it would be the case in this test. By default the tolerance is 1e-4:
>>> from sklearn.linear_model import Lasso
>>> Lasso().tol
0.0001This test does not override the default. The same is true for MultiTaskLasso.
There was a problem hiding this comment.
Ah ok I understand, this is because we rescale the tolerance by a data dependent quantity which is 0 in this case.
There was a problem hiding this comment.
That is indeed sub-optimal - y == 0 are valid options. What about the following change to keep tol as is if it would become 0 (similar code for all the different coordinate descent function instances):
--- a/sklearn/linear_model/_cd_fast.pyx
+++ b/sklearn/linear_model/_cd_fast.pyx
@@ -672,6 +672,7 @@ def enet_coordinate_descent_multi_task(
cdef floating W_ii_abs_max
cdef floating gap = tol + 1.0
cdef floating d_w_tol = tol
+ cdef floating temp_tol = tol
cdef floating R_norm
cdef floating w_norm
cdef floating ry_sum
@@ -704,7 +705,9 @@ def enet_coordinate_descent_multi_task(
&R[0, jj], 1)
# tol = tol * linalg.norm(Y, ord='fro') ** 2
- tol = tol * _nrm2(n_samples * n_tasks, Y_ptr, 1) ** 2
+ temp_tol = tol * _nrm2(n_samples * n_tasks, Y_ptr, 1) ** 2
+ if temp_tol > 0:
+ tol = temp_tol
for n_iter in range(max_iter):
w_max = 0.0
@@ -819,7 +822,7 @@ def enet_coordinate_descent_multi_task(
gap += l1_reg * l21_norm - const * ry_sum + \
0.5 * l2_reg * (1 + const ** 2) * (w_norm ** 2)
- if gap <= tol:
+ if gap < tol:
# return if we reached desired tolerance
break
else:
Then we could also return to the previous (gap < tol).
|
Hi @norbusan, thank you for your work so far! Are you still interested in finalizing this pull request? The list of the tests with convergence issues slightly changed. |
|
I gave this a fresh start @ogrisel @glemaitre . Had to remove the |
Reference Issues/PRs
closes #4780
What does this implement/fix? Explain your changes.
These warnings have been fixed by either increasing the number of
iterations of fitting, or by suppressing the warnings with pytest.warns
For parametrized tests a check is done so that only the failing tests
get the pytest.warns context manager, all other get a dummy context
manager that doesn't do anything.
Any other comments?
There is one instance where the warning does not disappear in
test_linear_model_sample_weights_normalize_in_pipeline
(second context manager call), but I don't know why ...