Skip to content
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

Update ngramizer.jl #148

Merged
merged 4 commits into from
May 3, 2019
Merged

Update ngramizer.jl #148

merged 4 commits into from
May 3, 2019

Conversation

djokester
Copy link
Contributor

The previous function returned all the ngrams from n to 1.
This error is also recorded in the documentation.
image

for index in 1:(n_words - n + 1)
token = join(words[index:(index + n - 1)], " ")
tokens[token] = get(tokens, token, 0) + 1

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also, imo the Travis CI is reporting an error due to change in implementation. Have a look at the tests corresponding to this change.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done Sir 💯

@Ayushk4
Copy link
Member

Ayushk4 commented Apr 26, 2019

Thank you so much for this. May you please, update the documents and add the tests as well?

Reflects changes made to ngrams() for n>1.
Reflects changes made to ngrams() for n greater than 1
@djokester
Copy link
Contributor Author

Updated Tests and Documentation to reflect the changes.

@aviks aviks merged commit 3b046d2 into JuliaText:master May 3, 2019
@djokester djokester deleted the patch-1 branch May 3, 2019 09:29
@tanmaykm
Copy link
Contributor

That could have been better implemented as an option IMO. Or by having a variant of ngrams that took a list of ngrams to generate. Useful while indexing for text search.

@Ayushk4
Copy link
Member

Ayushk4 commented May 17, 2019

We could open an issue for the same should be good first PR.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants