Repository: scikit-learn/scikit-learn
Pull request: FIX raise error for max_df and min_df greater than 1 in Vectorizer
<!--
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
<!--
Example: Fixes #1234. See also #3456.
Please use keywords (e.g., Fixes) to create link to the issues or pull requests
you resolved, so that they will automatically be closed when your pull request
is merged. See https://github.com/blog/1506-closing-issues-via-pull-requests
-->
Fixes #20746 

#### What does this implement/fix? Explain your changes.
When setting the min_idf and max_idf of CountVectorizer, throw an error if the value is not an int and bigger than 1. This reflects the range and functionality documented for these parameters.

#### Any other comments?


<!--
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:
Text vectorizer with unwanted float for max_idf or min_idf 
When working using CountVectorizer and accidentaly set max_df with float greater than 1.0 and give me wrong result. According to documentation we can set max_idf or min_idf with float [0,1] or int when using CountVectorizer.

#### Steps/Code to Reproduce

Taken from example:
```python

from sklearn.feature_extraction.text import CountVectorizer

corpus = [
    'This is the first document.',
    'This document is the second document.',
    'And this is the third one.',
    'Is this the first document?',
]

vectorizer = CountVectorizer(analyzer='word', min_df=2, max_df=3)
X = vectorizer.fit_transform(corpus)
print(vectorizer.get_feature_names())
# result ['document', 'first']
```

Suppose, I accidentaly set ```max_df``` with float greater than 1.0. This give me different result:

```python
vectorizer = CountVectorizer(analyzer='word', min_df=2, max_df=3.0)
X = vectorizer.fit_transform(corpus)
print(vectorizer.get_feature_names())
# result ['document', 'first', 'is', 'the', 'this']
```

Looking at the source, 
```python
   max_doc_count = (max_df
                             if isinstance(max_df, numbers.Integral)
                             else max_df * n_doc)
```
It seems  setting ```max_df=3.0``` will be multiplied by number of docs (4, in this case) and give same result as we directly set ```max_df=12```


#### Expected Results
Warning or a prevention for user if they set ```max_df``` or ```min_df``` with value that is not in range [0,1]

#### Versions
Linux-5.11.0-25-generic-x86_64-with-glibc2.10
Python 3.8.8 (default, Apr 13 2021, 19:58:26) 
[GCC 7.3.0]
NumPy 1.19.5
SciPy 1.6.2
Scikit-Learn 0.24.2
Imbalanced-Learn 0.7.0

Hints from issue discussion:
Thank you for your detailed bug report.

I was able to reproduce it and would like to contribute a PR. Is a warning sufficient or should it throw an error?

Commit message: changelog

Patch to review:
diff --git a/doc/whats_new/v1.0.rst b/doc/whats_new/v1.0.rst
index b4fcbee992383..3a6d6124feff4 100644
--- a/doc/whats_new/v1.0.rst
+++ b/doc/whats_new/v1.0.rst
@@ -366,6 +366,10 @@ Changelog
   error with unsupported value type.
   :pr:`19520` by :user:`Jeff Zhao <kamiyaa>`.
 
+- |Fix| Fixed a bug in :class:`feature_extraction.CountVectorizer` by raising an
+  error when min_idf or max_idf is a float and > 1.0.
+  :pr:`20752` by :user:`Alek Lefebvre <AlekLefebvre>`.
+
 :mod:`sklearn.feature_selection`
 ................................
 
diff --git a/sklearn/feature_extraction/tests/test_text.py b/sklearn/feature_extraction/tests/test_text.py
index 5b09325c61031..3cb20d6bb20f7 100644
--- a/sklearn/feature_extraction/tests/test_text.py
+++ b/sklearn/feature_extraction/tests/test_text.py
@@ -805,6 +805,28 @@ def test_vectorizer_min_df():
     assert len(vect.stop_words_) == 5
 
 
+def test_vectorizer_max_df_unwanted_float():
+    message = (
+        "max_df is out of range for a float value. Use an int or a float between 0.0"
+        " and 1.0."
+    )
+    with pytest.raises(ValueError, match=message):
+        test_data = ["abc", "dea", "eat"]
+        vect = CountVectorizer(analyzer="char", max_df=2.0)
+        vect.fit(test_data)
+
+
+def test_vectorizer_min_df_unwanted_float():
+    message = (
+        "min_df is out of range for a float value. Use an int or a float between 0.0"
+        " and 1.0."
+    )
+    with pytest.raises(ValueError, match=message):
+        test_data = ["abc", "dea", "eat"]
+        vect = CountVectorizer(analyzer="char", min_df=1.5)
+        vect.fit(test_data)
+
+
 def test_count_binary_occurrences():
     # by default multiple occurrences are counted as longs
     test_data = ["aaabc", "abbde"]
diff --git a/sklearn/feature_extraction/text.py b/sklearn/feature_extraction/text.py
index 24d9b07023fd3..d98386d03e209 100644
--- a/sklearn/feature_extraction/text.py
+++ b/sklearn/feature_extraction/text.py
@@ -1103,6 +1103,16 @@ def __init__(
         self.min_df = min_df
         if max_df < 0 or min_df < 0:
             raise ValueError("negative value for max_df or min_df")
+        if not isinstance(min_df, numbers.Integral) and min_df > 1.0:
+            raise ValueError(
+                "min_df is out of range for a float value. Use an int or a float"
+                " between 0.0 and 1.0."
+            )
+        if not isinstance(max_df, numbers.Integral) and max_df > 1.0:
+            raise ValueError(
+                "max_df is out of range for a float value. Use an int or a float"
+                " between 0.0 and 1.0."
+            )
         self.max_features = max_features
         if max_features is not None:
             if not isinstance(max_features, numbers.Integral) or max_features <= 0:


Code review comments:
diff_hunk: @@ -366,6 +366,10 @@ Changelog
   error with unsupported value type.
   :pr:`19520` by :user:`Jeff Zhao <kamiyaa>`.
 
+- |Fix| Fixed a bug in :class:`feature_extraction.CountVectorizer` by raising an
path: doc/whats_new/v1.0.rst
text: @user1:
```suggestion
- |Fix| Fixed a bug in :class:`feature_extraction.CountVectorizer` and
  :class:`feature_extraction.TfidfVectorizer` by raising an
```

Also please move this entry to v1.1.

diff_hunk: @@ -1103,6 +1103,16 @@ def __init__(
         self.min_df = min_df
         if max_df < 0 or min_df < 0:
             raise ValueError("negative value for max_df or min_df")
+        if not isinstance(min_df, numbers.Integral) and min_df > 1.0:
path: sklearn/feature_extraction/text.py
text: @user1:
I think we could do,
```suggestion
        if (not isinstance(min_df, numbers.Integral) and 
            (min_df < 1.0  or min_df > 1.0)):
```
and remove the above check since the error message there is not very helpful. Same for `max_df`. This might need adjusting tests though.

@author:
Hi @user1, thanks for your feedback! Do you mean to check min_df < **0**?

I'd be glad to make those changes but it's my understanding that we should still be checking for negative integer values and not just float.

We can remove the above check like you're suggesting but rewrite the new check this way instead:
```suggestion
        if min_df < 0 or
            (not isinstance(min_df, numbers.Integral) and min_df > 1.0):
```

Same for max_df.

I'd clarify the docs too, no negative integer is not explicitly mentioned.

Thanks!

@user1:
Yes, you are right the parenthesis was in the wrong place. Though yes, for an int the current message would not be ideal. OK, let's leave unchanged as it is.