Repository: scikit-learn/scikit-learn
Pull request: ENH add `only_non_negative` parameter to `_check_sample_weight`
<!--
Thanks for contributing a pull request! Please ensure you have taken a look at
the contribution guidelines: https://github.com/scikit-learn/scikit-learn/blob/main/CONTRIBUTING.md
-->

#### Reference Issues/PRs
Fixes #20036. 
Partially addressed https://github.com/scikit-learn/scikit-learn/issues/15531

#### What does this implement/fix? Explain your changes.
The LinearRegression() objects .fit() method gives unclear error message if the sample_weight parameter of the fit() method has negative value. The exception occured only at the call of linalg.lstsq(X, y). I think its better if the negative value check is done at the beginning of the .fit() method. The new message is: "The weights of the samples cannot be negative".

#### Any other comments?
I think the check for negative values should not be done inside the _check_sample_weight() function but inside the given .fit() method. Maybe there are other cases too where this exception message and the negative weight value check can be useful.

This is my first PR to the open source community and i wellcome any comment or advice related to this PR!


<!--
Please be aware that we are a loose team of volunteers so patience is
necessary; assistance handling other issues is very welcome. We value
all user contributions, no matter how minor they are. If we are slow to
review, either the pull request needs some benchmarking, tinkering,
convincing, etc. or more likely the reviewers are simply busy. In either
case, we ask for your understanding during the review process.
For more information, see our FAQ on this topic:
http://scikit-learn.org/dev/faq.html#why-is-my-pull-request-not-getting-any-attention.

Thanks for contributing!
-->


Problem statement:
Improve error message for negative values in sample_weight
With sklearn Version 0.22.1 a linear regression with negative sample weights fails with the following (unclear) error message.
Would be helpful if (e.g. `_check_sample_weight`) checks for negative weights and raises an appropriate error message.

```pytb
  File "C:\Users\gm889704\.conda\envs\gm\lib\site-packages\sklearn\pipeline.py", line 354, in fit
    self._final_estimator.fit(Xt, y, **fit_params)

  File "C:\Users\gm889704\.conda\envs\gm\lib\site-packages\sklearn\linear_model\_base.py", line 533, in fit
    linalg.lstsq(X, y)

  File "C:\Users\gm889704\.conda\envs\gm\lib\site-packages\scipy\linalg\basic.py", line 1157, in lstsq
    a1 = _asarray_validated(a, check_finite=check_finite)

  File "C:\Users\gm889704\.conda\envs\gm\lib\site-packages\scipy\_lib\_util.py", line 246, in _asarray_validated
    a = toarray(a)

  File "C:\Users\gm889704\.conda\envs\gm\lib\site-packages\numpy\lib\function_base.py", line 499, in asarray_chkfinite
    "array must not contain infs or NaNs")

ValueError: array must not contain infs or NaNs
```

Hints from issue discussion:
We could add a parameter `only_positive` that will be a boolean in `_check_sample_weight`.
By default, it would be False to be backward compatible and we can it to True and raise an error for negative weight in the estimator where it makes sense (I recall that we did something like this in `AdaBoost`).

Commit message: formatting with black .

Patch to review:
diff --git a/sklearn/linear_model/_base.py b/sklearn/linear_model/_base.py
index 79d6f321cb124..6e1d6acb0cc91 100644
--- a/sklearn/linear_model/_base.py
+++ b/sklearn/linear_model/_base.py
@@ -662,8 +662,10 @@ def fit(self, X, y, sample_weight=None):
             X, y, accept_sparse=accept_sparse, y_numeric=True, multi_output=True
         )
 
-        if sample_weight is not None:
-            sample_weight = _check_sample_weight(sample_weight, X, dtype=X.dtype)
+        if sample_weight is not None:  # the weights should be non-negative
+            sample_weight = _check_sample_weight(
+                sample_weight, X, only_non_negative=True, dtype=X.dtype
+            )
 
         X, y, X_offset, y_offset, X_scale = self._preprocess_data(
             X,
diff --git a/sklearn/utils/validation.py b/sklearn/utils/validation.py
index 871f4a191b2a9..554f821539dad 100644
--- a/sklearn/utils/validation.py
+++ b/sklearn/utils/validation.py
@@ -1479,7 +1479,9 @@ def _check_psd_eigenvalues(lambdas, enable_warnings=False):
     return lambdas
 
 
-def _check_sample_weight(sample_weight, X, dtype=None, copy=False):
+def _check_sample_weight(
+    sample_weight, X, only_non_negative=False, dtype=None, copy=False
+):
     """Validate sample weights.
 
     Note that passing sample_weight=None will output an array of ones.
@@ -1495,6 +1497,9 @@ def _check_sample_weight(sample_weight, X, dtype=None, copy=False):
     X : {ndarray, list, sparse matrix}
         Input data.
 
+    only_non_negative : if True then a non negativity check for the sample_weight
+        will be done.
+
     dtype : dtype, default=None
        dtype of the validated `sample_weight`.
        If None, and the input `sample_weight` is an array, the dtype of the
@@ -1540,6 +1545,9 @@ def _check_sample_weight(sample_weight, X, dtype=None, copy=False):
                 )
             )
 
+    if only_non_negative:
+        check_non_negative(sample_weight, "sample weight")
+
     return sample_weight
 
 


Code review comments:
diff_hunk: @@ -1479,7 +1479,9 @@ def _check_psd_eigenvalues(lambdas, enable_warnings=False):
     return lambdas
 
 
-def _check_sample_weight(sample_weight, X, dtype=None, copy=False):
+def _check_sample_weight(
+    sample_weight, X, only_non_negative=False, dtype=None, copy=False
path: sklearn/utils/validation.py
text: @user1:
Here there is a silent bug. You need to put `only_non_negative` as the last argument because some call of `_check_sample_weight` are using positional argument. Thus, passing previously the dtype as a positional argument become the value of `only_non_negative`.

diff_hunk: @@ -1495,6 +1497,9 @@ def _check_sample_weight(sample_weight, X, dtype=None, copy=False):
     X : {ndarray, list, sparse matrix}
         Input data.
 
+    only_non_negative : if True then a non negativity check for the sample_weight
+        will be done.
path: sklearn/utils/validation.py
text: @user1:
```suggestion
    only_non_negative : bool, default=False,
        Whether or not the weights are expected to be non-negative.
        
        .. versionadded:: 1.0
```

diff_hunk: @@ -662,8 +662,10 @@ def fit(self, X, y, sample_weight=None):
             X, y, accept_sparse=accept_sparse, y_numeric=True, multi_output=True
         )
 
-        if sample_weight is not None:
-            sample_weight = _check_sample_weight(sample_weight, X, dtype=X.dtype)
+        if sample_weight is not None:  # the weights should be non-negative
path: sklearn/linear_model/_base.py
text: @user1:
There is no need for this comment since passing the keyword argument is self-explanatory.

```suggestion
        if sample_weight is not None:
```

diff_hunk: @@ -1540,6 +1545,9 @@ def _check_sample_weight(sample_weight, X, dtype=None, copy=False):
                 )
             )
 
+    if only_non_negative:
+        check_non_negative(sample_weight, "sample weight")
path: sklearn/utils/validation.py
text: @user1:
```suggestion
        check_non_negative(sample_weight, "`sample_weight`")
```