Repository: scikit-learn/scikit-learn
Pull request: [MRG] Add codes to raise error when l1_ratio is out-of-range in elasticnet
#### Reference Issues/PRs

Fixes #17814 

#### What does this implement/fix? Explain your changes.

This PR added codes (mainly copied from `sklearn/linearmodel/_logistic.py`) to raise `ValueError` in the Elasticnet implementation in `sklearn/linearmodel/_coordinate_descent.py` when the range is outside of `0<l1_ratio<1` or not numeric.  I also added a unit test in `sklearn/linearmodel/test/test_coordinate_descent.py` following similar test in `sklearn/linearmodel/test/test_logistic.py` called `test_l1_ratio_param` which exactly tests the raised message.

#### Any other comments?

There might be other functions/estimators which take `l1_ratio` as input (as raised by @rth in #17814) which should have similar `ValueError` raised. If I found such needs/issues, I'll raise another issue thread after I systematically went through the codebase for that.


Problem statement:
l1_ratio in sklearn.linear_model's ElasticNet greater than 1?
I accidentally ran ElasticNet (from sklearn.linear_model) for l1_ratio >1, and no error or warning was raised. From the docsstring, it says that ``0 < l1_ratio < 1``. Should we raise a ValueError or something? Found this with @mathurinm.

If this turns out to be something to be done, I could help out if someone could point me towards the right direction. Thanks !

p/s: Not sure if this should be under bugs/documentations/others, so I listed it under others. Sklearn version is 0.22.1.

Hints from issue discussion:
Yes, as far as I can tell there is no input validation of the `l1_ratio` range. A PR would be very welcome. Just please check if it's also an issue for other public functions/estimators that take `l1_ratio` as input (which also might need fixing).
@rth Thanks for the comment. It turns out that in ``sklearn/linearmodel/_logistic.py``, there is a  code that deals with this ``ValueError``: 
```
            if (not isinstance(self.l1_ratio, numbers.Number) or
                    self.l1_ratio < 0 or self.l1_ratio > 1):
                raise ValueError("l1_ratio must be between 0 and 1;"
                                 " got (l1_ratio=%r)" % self.l1_ratio)
```

Furthermore, there is a function in ``sklearn/linearmodel/test/test_logistic.py`` called
``test_l1_ratio_param`` which tests that. I'm going to copy (and modify appropriately) both of these (the error raising and testing) to the Elasticnet implementation in ``sklearn/linearmodel/_coordinate_descent.py`` and create a test in ``sklearn/linearmodel/test/test_coordinate_descent.py``

At this point, I will do this for elastic net first and will create a PR soon to resolve this. After that, I will look through to see if other functions have similar concerns regarding range of ``l1_ratio`` and create another issue ticket for that. It might take me a little more time since it involves a sweep over the (quite large) code base.

Commit message: Update sklearn/linear_model/tests/test_coordinate_descent.py

Co-authored-by: Nicolas Hug <contact@nicolas-hug.com>

Patch to review:
diff --git a/sklearn/linear_model/_coordinate_descent.py b/sklearn/linear_model/_coordinate_descent.py
index 570d048de1e64..be60de2354ace 100644
--- a/sklearn/linear_model/_coordinate_descent.py
+++ b/sklearn/linear_model/_coordinate_descent.py
@@ -755,6 +755,11 @@ def fit(self, X, y, sample_weight=None, check_input=True):
             raise ValueError('precompute should be one of True, False or'
                              ' array-like. Got %r' % self.precompute)
 
+        if (not isinstance(self.l1_ratio, numbers.Number) or
+                self.l1_ratio < 0 or self.l1_ratio > 1):
+            raise ValueError("l1_ratio must be between 0 and 1; "
+                             f"got l1_ratio={self.l1_ratio}")
+
         # Remember if X is copied
         X_copied = False
         # We expect X and y to be float64 or float32 Fortran ordered arrays
diff --git a/sklearn/linear_model/tests/test_coordinate_descent.py b/sklearn/linear_model/tests/test_coordinate_descent.py
index 4963444300552..2d0456d93310e 100644
--- a/sklearn/linear_model/tests/test_coordinate_descent.py
+++ b/sklearn/linear_model/tests/test_coordinate_descent.py
@@ -58,6 +58,19 @@
 from sklearn.utils import check_array
 
 
+@pytest.mark.parametrize('l1_ratio', (-1, 2, None, 10, 'something_wrong'))
+def test_l1_ratio_param_invalid(l1_ratio):
+    # Check that correct error is raised when l1_ratio in ElasticNet
+    # is outside the correct range
+    X = np.array([[-1.], [0.], [1.]])
+    Y = [-1, 0, 1]       # just a straight line
+
+    msg = "l1_ratio must be between 0 and 1; "f"got l1_ratio={l1_ratio}"
+    clf = ElasticNet(alpha=0.1, l1_ratio=l1_ratio)
+    with pytest.raises(ValueError, match=msg):
+        clf.fit(X, Y)
+
+
 @pytest.mark.parametrize('order', ['C', 'F'])
 @pytest.mark.parametrize('input_order', ['C', 'F'])
 def test_set_order_dense(order, input_order):


Code review comments:
diff_hunk: @@ -58,6 +58,19 @@
 from sklearn.utils import check_array
 
 
+@pytest.mark.parametrize('l1_ratio', (-1, 2, None, 10, 'something_wrong'))
+def test_l1_ratio_param_invalid(l1_ratio):
+    # Check that correct error is raised when l1_ratio in ElasticNet
+    # is outside the correct range
+    X = np.array([[-1.], [0.], [1.]])
+    Y = [-1, 0, 1]       # just a straight line
+
+    msg = "l1_ratio must be between 0 and 1; "f"got l1_ratio={l1_ratio}"
path: sklearn/linear_model/tests/test_coordinate_descent.py
text: @user1:
```suggestion
    msg = "l1_ratio must be between 0 and 1; got l1_ratio={l1_ratio}"
```