Repository: scikit-learn/scikit-learn
Pull request: API Raises an error when a negative threshold is chosen in VarianceThreshold
<!--
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 #20206


#### What does this implement/fix? Explain your changes.

I made a quick fix of an unexpected behavior of `VarianceThreshold` when a negative value of threshold is chosen.
And I added a test code related to this problem.



Problem statement:
Unexpected behavior of VarianceThreshold when a threshold is negative.
#### Describe the bug

When a threshold is negative, an unexpected result returns.
For now, I'm not sure why this happens.

#### Steps/Code to Reproduce



Example:
```python
data = [[0, 1, 2, 3, 4],
        [0, 2, 2, 3, 5],
        [1, 1, 2, 4, 0]]

  for X in [data, csr_matrix(data)]:
      X_neg = VarianceThreshold(threshold=-1.).fit_transform(X)
      X_zero = VarianceThreshold(threshold=0.).fit_transform(X)
      assert X_neg.shape == X_zero.shape
```


#### Expected Results
`(3, 4) == (3, 4)`

#### Actual Results
`(3, 5) == (3, 4)`

#### Versions

numpy --> 1.16.5
scipy --> 1.3.1

Hints from issue discussion:

Commit message: Add whats new

Patch to review:
diff --git a/doc/whats_new/v1.0.rst b/doc/whats_new/v1.0.rst
index 02c77459ddc22..7b743255618c1 100644
--- a/doc/whats_new/v1.0.rst
+++ b/doc/whats_new/v1.0.rst
@@ -295,6 +295,10 @@ Changelog
   :pr:`17169` by :user:`Dmytro Lituiev <DSLituiev>`
   and :user:`Julien Jerphanion <jjerphan>`.
 
+- |Fix| Fixed a bug in class:`feature_selection.VarianceThreshold`
+  when a variance threshold is negative.
+  :pr:`20207` by :user:`Tomohiro Endo <europeanplaice>`
+
 :mod:`sklearn.inspection`
 .........................
 
diff --git a/sklearn/feature_selection/_variance_threshold.py b/sklearn/feature_selection/_variance_threshold.py
index 39892876a6478..ea7f3411de7e8 100644
--- a/sklearn/feature_selection/_variance_threshold.py
+++ b/sklearn/feature_selection/_variance_threshold.py
@@ -84,6 +84,10 @@ def fit(self, X, y=None):
             # for constant features
             compare_arr = np.array([self.variances_, peak_to_peaks])
             self.variances_ = np.nanmin(compare_arr, axis=0)
+        elif self.threshold < 0.:
+            raise ValueError(
+                "Threshold must be non-negative."
+                f" Got: {self.threshold}")
 
         if np.all(~np.isfinite(self.variances_) |
                   (self.variances_ <= self.threshold)):
diff --git a/sklearn/feature_selection/tests/test_variance_threshold.py b/sklearn/feature_selection/tests/test_variance_threshold.py
index 23e7703708984..6eff920a66192 100644
--- a/sklearn/feature_selection/tests/test_variance_threshold.py
+++ b/sklearn/feature_selection/tests/test_variance_threshold.py
@@ -13,6 +13,7 @@
 
 data2 = [[-0.13725701]] * 10
 
+
 def test_zero_variance():
     # Test VarianceThreshold with default setting, zero variance.
 
@@ -33,6 +34,16 @@ def test_variance_threshold():
         assert (len(data), 1) == X.shape
 
 
+@pytest.mark.parametrize('X', [data, csr_matrix(data)])
+def test_variance_negative(X):
+    """Test VarianceThreshold with negative variance."""
+    var_threshold = VarianceThreshold(threshold=-1.)
+    msg = '^Threshold must be non-negative.' \
+          ' Got: -1.0$'
+    with pytest.raises(ValueError, match=msg):
+        var_threshold.fit(X)
+
+
 @pytest.mark.skipif(np.var(data2) == 0,
                     reason=('This test is not valid for this platform, '
                             'as it relies on numerical instabilities.'))


Code review comments:
diff_hunk: @@ -295,6 +295,10 @@ Changelog
   :pr:`17169` by :user:`Dmytro Lituiev <DSLituiev>`
   and :user:`Julien Jerphanion <jjerphan>`.
 
+- |Fix| Fixed a bug in class:`feature_selection.VarianceThreshold`
+  when a variance threshold is negative.
path: doc/whats_new/v1.0.rst
text: @user1:
To properly link the class and a minor adjustment to the text:

```suggestion
- |API| Raises an error in :class:`feature_selection.VarianceThreshold`
  when the variance threshold is negative.
```

Looking at this again, I think this is more of an API change where we are performing more validation.