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

Added a distance method to the Glove class #19

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

owo
Copy link

@owo owo commented Dec 9, 2014

Added a distance method to the Glove class to measure the distance between two arbitrary words.
I needed the functionality for my own project and thought it would be useful for this project as a whole.
Let me know if there are any other changes that may need to be done.

@maciejkula
Copy link
Owner

Looks good at first glance. I'll merge when I have access to a computer.
On 10 Dec 2014 00:02, "Ossama W. Obeid" notifications@github.com wrote:

Added a distance method to the Glove class to measure the distance between
two arbitrary words.
I needed the functionality for my own project and thought it would be
useful for this project as a whole.

Let me know if there are any other changes that may need to be done.

You can merge this Pull Request by running

git pull https://github.com/owo/glove-python master

Or view, comment on, or merge it at:

#19
Commit Summary

  • Added a distance method to the Glove class to measure the distance
    between two arbitrary words.

File Changes

Patch Links:


Reply to this email directly or view it on GitHub
#19.

@maciejkula
Copy link
Owner

A couple of comments:

  1. Could you encapsulate checking whether the model has been fitted into its own method so that we could reuse it in both your new method and most_similar? You could call this _check_model_fitted or something along those lines.
  2. If I understand correctly, what you are actually returning is the similarity of two words, not their distance. The distance would be 1 - that? You could rename your method to similarity?

@owo
Copy link
Author

owo commented Dec 20, 2014

Sure thing. I'll commit the changes as soon as I can.

@owo
Copy link
Author

owo commented Dec 20, 2014

@maciejkula one question. Would you like to have _check_model_fitted to check if a word dictionary has been provided as well?

@maciejkula
Copy link
Owner

Good question. Maybe just leave the checks as you wrote them for the moment and I will have a look later and see if there's a non-awful solution.

@owo
Copy link
Author

owo commented Dec 26, 2014

Okay, so I decided to retain the distance function for now, by subtracting from 1 the computed similarity.
I don't know, distance seems more useful (or intuitive, rather) than similarity when comparing two particular words (at least for my use cases).

@maciejkula
Copy link
Owner

I must say I prefer to have the similarity function, it seems to me to be more consistent with the most_similar method.

@owo
Copy link
Author

owo commented Jan 1, 2015

Done :)

except KeyError:
raise Exception('Word not in dictionary')

return self._distance(self.word_vectors[word1_idx],
Copy link
Owner

Choose a reason for hiding this comment

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

Does this actually work? Maybe a leftover from the previous version?

Copy link

@enfageorge enfageorge Aug 30, 2018

Choose a reason for hiding this comment

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

@owo _distance is not defined in the current version anyway. I tried running the code with the added functions _similarity and similarity but ended up with
AttributeError: 'Glove' object has no attribute '_distance'

@maciejkula
Copy link
Owner

We can address the KeyError thing in a separate PR (but I agree that a KeyError is probably better).

Could you just check my comment above? And then maybe I'll finally merge!

@theeluwin
Copy link

👍

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.

5 participants