-
Notifications
You must be signed in to change notification settings - Fork 908
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
Expose stream parameter in public nvtext ngram APIs #14061
Expose stream parameter in public nvtext ngram APIs #14061
Conversation
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.
Looks good, but same request for tests as on #14060
@vyasr How do I run these tests locally? |
The easiest way to run the test with the appropriate configuration is to use ctest.
|
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.
LGTM. I'm changing the labels to mark this PR as breaking now though since the separator is no longer optional.
/merge |
Description
Add stream parameter to public APIs:
nvtext::generate_ngrams()
nvtext::generate_character_ngrams()
nvtext::hash_character_ngrams()
nvtext::ngrams_tokenize()
Also cleaned up some of the doxygen comments.
And also fixed a spelling mistake in the jaccard.cu source that was bothering me.
Reference #13744
Checklist