Repository: scikit-learn/scikit-learn
Pull request: FIX raise an error in CountVectorizer with a custom token pattern that captures several group
<!--
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/master/CONTRIBUTING.md#pull-request-checklist
-->

#### 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 #12971 

#### What does this implement/fix? Explain your changes.
1. Merges in changes from #13229, which raises a ValueError if the token pattern regex has more than one capturing group and documents the general behavior of capturing groups in the token pattern regex
2. Improves the doc for token_pattern per requested changes on #13229
3. Changes the test spec from testing 0 capturing groups to testing 1 capturing group per requested changes on #13229
4. Modifies the test corpus and regexes for `test_countvectorizer_custom_token_pattern` so they represent a more "realistic" type of custom token pattern than used in #13229.  The previous test used an example from #12971 where the custom token pattern behaved the same as the default token pattern on that particular corpus.  My new test is an adapted version of those examples that uses some of the same selectors but also (a) behaves differently than the default and (b) gives an example of what you might actually use a capturing group for.  I have modified both regexes so they meet these requirements and are almost identical, but the first has 1 capturing group and the second has 2 capturing groups.

#### Any other comments?
Same as #13229, this does not address deprecation or backward compatibility

<!--
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:
CountVectorizer token_pattern issue with multi Alternative regex pattern.
#### Description
When using the custom token_pattern with CountVectorize returns no feature names. Am i missing something or 

#### Steps/Code to Reproduce
```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(token_pattern=r"[a-z]{2,}|([0-9]{1,3})(?:st|nd|rd|th)?") # Trying to use the custom-designed regex pattern, which matches the tokens as per our needs.
X = vectorizer.fit_transform(corpus)
print(vectorizer.get_feature_names())
```
#### Expected Results
```shell
['and', 'document', 'first', 'is', 'one', 'second', 'the', 'third', 'this']
```

#### Actual Results
```shell
 [''] 
```

#### Alrternate Code but not using Scikit
i tried the same pattern with a simple regex tokenizer and it works smooth.

```python
import re
def regex_tokenizer(corpus):
    scanner = re.compile(r"[a-z]{2,}|([0-9]{1,3})(?:st|nd|rd|th)?")
    return [match.group() for match in re.finditer(scanner, corpus)]


corpus = 'This is the first document.'
print(regex_tokenizer(corpus))
```

####Results as Expected.
```shell
['his', 'is', 'the', 'first', 'document']
```

#### Versions
For scikit-learn >= 0.20.1:

Hints from issue discussion:
It seems we have a bug (or feature) through the use of `re.findall`. The contents of the capturing group `([0-9]{1,3})` is being returned as the token. Make it non-capturing `(?:[0-9]{1,3})` as one solution. I think this is probably historically a bug, but whether users have exploited it is hard to tell, so we may not be able to easily fix it in a backwards-compatible way.
@jnothman thanks the non-capturing works for me.
The number of capturing groups can be counted with the `.groups` attribute of a compiled pattern.

I think we should:
* change the code to raise an error if the number of groups is greater than 1, since this wouldn't work atm (I think?)
* document the behaviour when there is a capturing group (i.e. the captured group content, not the entire match, becomes the token)
* ?deprecate this support for a capturing group
Hello @jnothman, Can I work on this?
Sure @souvik3333, thanks.
I was wondering if this issue has been solved? Can I work on this? @souvik3333 @rth 
@FangzhengHu , I don't think there is any PR related to it for now. 
Agreed. I haven't seen a PR for it, so if someone want to take it, just say it a comment. 
@jnothman
> The number of capturing groups can be counted with the `.groups` attribute of a compiled pattern.
> 
> I think we should:
> 
> * change the code to raise an error if the number of groups is greater than 1, since this wouldn't work atm (I think?)  
- Can't there be more than one capturing group, to give more flexibility to users using regex?
> * document the behaviour when there is a capturing group (i.e. the captured group content, not the entire match, becomes the token) 
> * ?deprecate this support for a capturing group

It come downs if we want to consider capturing group as a single token or just use  entire match as a token. In First, allows multiple tokens from single match and add it to the token list.In second,  as it is. 



I am working on this

Commit message: updating CountVectorizer and TfidfVectorizer docs to match

Patch to review:
diff --git a/sklearn/feature_extraction/tests/test_text.py b/sklearn/feature_extraction/tests/test_text.py
index 25c353aae5276..069876521b86f 100644
--- a/sklearn/feature_extraction/tests/test_text.py
+++ b/sklearn/feature_extraction/tests/test_text.py
@@ -351,6 +351,30 @@ def test_fit_countvectorizer_twice():
     assert X1.shape[1] != X2.shape[1]
 
 
+def test_countvectorizer_custom_token_pattern():
+    corpus = [
+        'This is the 1st document in my corpus.',
+        'This document is the 2nd sample.',
+        'And this is the 3rd one.',
+        'Is this the 4th document?',
+    ]
+    # With 1 capturing group, capturing the word after an ordinal
+    token_pattern = r"[0-9]{1,3}(?:st|nd|rd|th)\s\b(\w{2,})\b"
+    vectorizer = CountVectorizer(token_pattern=token_pattern)
+    vectorizer.fit_transform(corpus)
+    expected = ['document', 'one', 'sample']
+    assert vectorizer.get_feature_names() == expected
+    # With 2 capturing groups, capturing the ordinal and the word after it
+    token_pattern = r"([0-9]{1,3}(?:st|nd|rd|th))\s\b(\w{2,})\b"
+    message = "more than 1 capturing group in token pattern"
+
+    def func(token_pattern):
+        vectorizer = CountVectorizer(token_pattern=token_pattern)
+        vectorizer.fit_transform(corpus)
+
+    assert_raise_message(ValueError, message, func, token_pattern)
+
+
 def test_tf_idf_smoothing():
     X = [[1, 1, 1],
          [1, 1, 0],
diff --git a/sklearn/feature_extraction/text.py b/sklearn/feature_extraction/text.py
index dd293531184c3..b556e06a0fe78 100644
--- a/sklearn/feature_extraction/text.py
+++ b/sklearn/feature_extraction/text.py
@@ -324,6 +324,10 @@ def build_tokenizer(self):
         if self.tokenizer is not None:
             return self.tokenizer
         token_pattern = re.compile(self.token_pattern)
+
+        if token_pattern.groups > 1:
+            raise ValueError("more than 1 capturing group in token pattern")
+
         return token_pattern.findall
 
     def get_stop_words(self):
@@ -608,6 +612,10 @@ class HashingVectorizer(TransformerMixin, _VectorizerMixin, BaseEstimator):
         or more alphanumeric characters (punctuation is completely ignored
         and always treated as a token separator).
 
+        If there is a capturing group in token_pattern then the
+        captured group content, not the entire match, becomes the token.
+        At most one capturing group is permitted.
+
     ngram_range : tuple (min_n, max_n), default=(1, 1)
         The lower and upper boundary of the range of n-values for different
         n-grams to be extracted. All values of n such that min_n <= n <= max_n
@@ -874,6 +882,10 @@ class CountVectorizer(_VectorizerMixin, BaseEstimator):
         or more alphanumeric characters (punctuation is completely ignored
         and always treated as a token separator).
 
+        If there is a capturing group in token_pattern then the
+        captured group content, not the entire match, becomes the token.
+        At most one capturing group is permitted.
+
     ngram_range : tuple (min_n, max_n), default=(1, 1)
         The lower and upper boundary of the range of n-values for different
         n-grams to be extracted. All values of n such that min_n <= n <= max_n
@@ -1572,6 +1584,10 @@ class TfidfVectorizer(CountVectorizer):
         or more alphanumeric characters (punctuation is completely ignored
         and always treated as a token separator).
 
+        If there is a capturing group in token_pattern then the
+        captured group content, not the entire match, becomes the token.
+        At most one capturing group is permitted.
+
     ngram_range : tuple (min_n, max_n), default=(1, 1)
         The lower and upper boundary of the range of n-values for different
         n-grams to be extracted. All values of n such that min_n <= n <= max_n


Code review comments:
diff_hunk: @@ -351,6 +351,30 @@ def test_fit_countvectorizer_twice():
     assert X1.shape[1] != X2.shape[1]
 
 
+def test_countvectorizer_custom_token_pattern():
+    corpus = [
+        'This is the 1st document in my corpus.',
+        'This document is the 2nd sample.',
+        'And this is the 3rd one.',
+        'Is this the 4th document?',
+    ]
+    # With 1 capturing group, capturing the word after an ordinal
+    token_pattern = r"[0-9]{1,3}(?:st|nd|rd|th)\s\b(\w{2,})\b"
+    vectorizer = CountVectorizer(token_pattern=token_pattern)
+    vectorizer.fit_transform(corpus)
+    expected = ['document', 'one', 'sample']
+    assert vectorizer.get_feature_names() == expected
+    # With 2 capturing groups, capturing the ordinal and the word after it
+    token_pattern = r"([0-9]{1,3}(?:st|nd|rd|th))\s\b(\w{2,})\b"
+    message = "more than 1 capturing group in token pattern"
+
+    def func(token_pattern):
+        vectorizer = CountVectorizer(token_pattern=token_pattern)
+        vectorizer.fit_transform(corpus)
+
+    assert_raise_message(ValueError, message, func, token_pattern)
path: sklearn/feature_extraction/tests/test_text.py
text: @user1:
We usually try to test a more minimal context that will error. That is, rather than func here, we would have vectorizer.fit