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

Wrong distance in Lexical entailment experiment for Poincare embeddings #1858

Closed
octavian-ganea opened this issue Jan 25, 2018 · 5 comments
Closed
Labels
bug Issue described a bug difficulty easy Easy issue: required small fix good first issue Issue for new contributors (not required gensim understanding + very simple)

Comments

@octavian-ganea
Copy link

The following line https://github.com/RaRe-Technologies/gensim/blob/develop/gensim/models/poincare.py#L1522
should be:

return -1 * (1 + self.alpha * (norm_2 - norm_1)) * min_distance

otherwise distance is the last encountered distance, not the smallest one.

@menshikh-iv
Copy link
Contributor

Thanks for report @octavian-ganea, looks like a typo (that generates bug here).
@jayantj can you confirm?

@jayantj
Copy link
Contributor

jayantj commented Jan 26, 2018

Yeah, that looks like a bug. Good catch.

@menshikh-iv menshikh-iv added bug Issue described a bug difficulty easy Easy issue: required small fix good first issue Issue for new contributors (not required gensim understanding + very simple) labels Jan 26, 2018
@ghost
Copy link

ghost commented Jan 28, 2018

Hi, Can I take this one?

@menshikh-iv
Copy link
Contributor

@hachibaka of course, feel free to contribute!

@ghost
Copy link

ghost commented Jan 30, 2018

completed, PR #1863.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Issue described a bug difficulty easy Easy issue: required small fix good first issue Issue for new contributors (not required gensim understanding + very simple)
Projects
None yet
Development

No branches or pull requests

3 participants