-
Notifications
You must be signed in to change notification settings - Fork 526
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[REVIEW]Hashing Vectorizer and general vectorizer improvements #2554
[REVIEW]Hashing Vectorizer and general vectorizer improvements #2554
Conversation
Please update the changelog in order to start CI tests. View the gpuCI docs here. |
This should now be ready for a first review. |
" is not supported in cuML," | ||
" please read the cuML documentation for" | ||
" more information.") | ||
class HashingVectorizer(_VectorizerMixin): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Main Logic Starts here
@@ -602,15 +633,248 @@ def get_feature_names(self): | |||
""" | |||
return self.vocabulary_ | |||
|
|||
def _check_sklearn_params(self, analyzer, sklearn_params): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Moved this to base class _VectorizerMixin
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In a quick first review I have mainly comments regarding docstrings and using the cuml logger, nothing major, will do another pass tomorrow.
@@ -51,15 +49,23 @@ def _preprocess(doc, lower=False, remove_non_alphanumeric=False, | |||
if lower: | |||
doc = doc.str.lower() | |||
if remove_non_alphanumeric: | |||
non_alpha = _get_non_alphanumeric_characters(doc) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
all non_alpha chars excluding _
char, ^\w
does not extract _
.
Co-authored-by: Dante Gama Dessavre <dante.gamadessavre@gmail.com>
Ready for the second round of reviews. |
Just linking the test/benchmarking notebooks here:
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
3 super small comments/question, but overall looks great (and worked well in my initial testings)! Awesome job @VibhuJawa
Co-authored-by: Dante Gama Dessavre <dante.gamadessavre@gmail.com>
Co-authored-by: Dante Gama Dessavre <dante.gamadessavre@gmail.com>
…to fea-hashing-vectorizer
All reviews have been addressed, Should be good for a final review now I think. |
I just realized rapidsai/cudf#5658 is also in, will integrate that in a follow-up pr. |
Closed last remaining bits of #1266 . |
This pr adds
Hashing Vectorizer
and improvesCountVectorizer
Count Vectorizer Improvements
All benchmarks on a
Tesla T4
and dataset used is the one used in the blogImproves memory pressures
Improves Run Time(3x improvement)
Hash Vectorizer: Benchmarks (9.5x speed up)
Known Divergence (NOT INTRODUCED BY THIS PR): https://gist.github.com/VibhuJawa/5f8e2666da9ecbb63d0398277fc93ac5
Just linking the test/benchmarking notebooks here:
Doc Search using Hash-Vectorizer (Gets relevant same results as count-vectorizer as on the blog)
PR performance Notebook
Master Performance Notebook