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

Fixed Typo and increased performance in analyze_sentence #2070

Merged
merged 2 commits into from
Jun 10, 2018
Merged

Fixed Typo and increased performance in analyze_sentence #2070

merged 2 commits into from
Jun 10, 2018

Conversation

JonathanHourany
Copy link
Contributor

Type on line 147. Sentence is "...happy ending has it won't..." where "has" should be "as"

Type on  line 147. Sentence is "...happy ending has it won't..." where "has" should be "as"
@piskvorky
Copy link
Owner

Thanks @JonathanHourany . Looking at the code, that whole "trick" is questionable though -- adding two lists together will create a new list, which is a very costly operation in such a tight spot.

So I don't think that's a good idea in the first place, but we wanted to replace that code by an optimized Cython loop anyway…

@JonathanHourany
Copy link
Contributor Author

I agree, seems it'd be better to append None instead. If a Cython implementation is far off I could make the change in a new commit if you feel it's worth it.

@piskvorky
Copy link
Owner

Yes please!

As @piskvorky pointed out, using `s + [None]`on line 149 creates a new list, an expensive operation considering the goal is just to append `None` to an already existing list. Using `append` increases performance by 5 micro seconds per function call on my system
@JonathanHourany JonathanHourany changed the title Typo Fixed Typo and increased performance in analyze_sentence Jun 9, 2018
@piskvorky piskvorky merged commit e50a057 into piskvorky:develop Jun 10, 2018
@piskvorky
Copy link
Owner

piskvorky commented Jun 10, 2018

Thanks @JonathanHourany ! Congrats on your first Gensim contribution 🏅

@JonathanHourany
Copy link
Contributor Author

Thank you!

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.

2 participants