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

fix _get_word_ngrams() #84

Closed
wants to merge 1 commit into from
Closed

fix _get_word_ngrams() #84

wants to merge 1 commit into from

Conversation

odek53r
Copy link
Contributor

@odek53r odek53r commented Feb 26, 2017

Hi,
I get a wrong tuple, ('test','This'), in the results, when inputting two sentences, 'This is a test' and 'This is a contest', to the _get_word_ngrams(2, sentences). The expected results are ('This','is'), ('is','a'), ('a','test'), ('a','contest').

default

The image clearly shows debug results when running test_get_word_ngrams() using PyCharm.

The reason to the mistake results is that _get_word_ngrams() concatenates all words of sentences and then calls _get_ngrams(n, words).

I correct _get_word_ngrams() code that the sentences will not be concatenated.

@miso-belica
Copy link
Owner

miso-belica commented Mar 4, 2017

Hi, thanks for this. Can you squash these commits into one and add new test instead of modifying existing one please? Also please rebase your branch to current master - I fixed failing tests for Python 3.3. there. Thanks.

@miso-belica miso-belica added the bug label Mar 4, 2017
@miso-belica miso-belica self-assigned this Mar 4, 2017
@miso-belica miso-belica changed the base branch from dev to master March 4, 2017 11:33
@miso-belica miso-belica self-requested a review March 4, 2017 11:33
@miso-belica miso-belica assigned odek53r and unassigned miso-belica Mar 4, 2017
@odek53r odek53r force-pushed the dev branch 2 times, most recently from 0998a15 to 052ef48 Compare March 4, 2017 19:00
@odek53r
Copy link
Contributor Author

odek53r commented Mar 4, 2017

done

@miso-belica
Copy link
Owner

Thanks, merged in bc489c6 with some tweaks :)

@miso-belica miso-belica closed this Mar 5, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants