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

Add build_vocab to poincare model #2505

Merged

Conversation

Projects
None yet
3 participants
@koiizukag
Copy link
Contributor

commented May 29, 2019

This PR makes the following changes: to fix #2514

  • Add build_vocab to poincare model for online learning.
  • add unit test.
@koiizukag

This comment has been minimized.

Copy link
Contributor Author

commented May 31, 2019

Add issue #2514 😃

@koiizukag

This comment has been minimized.

Copy link
Contributor Author

commented Jun 3, 2019

@piskvorky Hi, please assign some reviewer

@piskvorky

This comment has been minimized.

Copy link
Member

commented Jun 3, 2019

We're busy finishing up a large "Gensim docs" project, but @mpenkov will get to your PR eventually. Thanks for your patience @koiizukag.

@mpenkov mpenkov self-assigned this Jun 21, 2019

@mpenkov
Copy link
Collaborator

left a comment

Thank you for your patience. Your changes look good overall, but I left you some minor comments.

Show resolved Hide resolved gensim/models/poincare.py

def _load_relations(self):
def build_vocab(self, relations=None, update=False):

This comment has been minimized.

Copy link
@mpenkov

mpenkov Jun 21, 2019

Collaborator

You've made this a public method, so it needs a good docstring. Please look at other docstrings in the code for examples on how to write a good one.

Show resolved Hide resolved gensim/models/poincare.py

koiizukag added some commits Jun 21, 2019

fix TypeError
add doc
Merge branch 'add-build_vocab-to-poincare_model' of github.com:koiizu…
…kag/gensim into add-build_vocab-to-poincare_model
relations : list of tuples
List of tuples of positive examples of the form (node_1_index, node_2_index).
update : bool
If true, the new nodes in `relations` will be added to model's vocab.

This comment has been minimized.

Copy link
@piskvorky

piskvorky Jun 21, 2019

Member

But relations doesn't contain nodes. Its description above says it contains "node indexes" (btw where does the user find those?).

Also, what happens if False (the default)?

This comment has been minimized.

Copy link
@koiizukag

koiizukag Jun 21, 2019

Author Contributor

I modified the description of relations using init description.

But relations doesn't contain nodes. Its description above says it contains "node indexes" (btw where does the user find those?).

If update=False, the embeddings are initialized by random values.
(It means that the trained embeddings are cleaned.)

Also, what happens if False (the default)?

This comment has been minimized.

Copy link
@piskvorky

piskvorky Jun 21, 2019

Member

OK, I don't really understand that, but that information should appear in the documentation.

This comment has been minimized.

Copy link
@koiizukag

koiizukag Jun 21, 2019

Author Contributor

I added update=False description in build_vocab 👍

@mpenkov
Copy link
Collaborator

left a comment

Very good. I'm going to suggest some very minor corrections for clarity. Please have a look and apply them if they make sense.

Show resolved Hide resolved gensim/models/poincare.py Outdated
Show resolved Hide resolved gensim/models/poincare.py Outdated
Show resolved Hide resolved gensim/models/poincare.py Outdated
Show resolved Hide resolved gensim/models/poincare.py
Show resolved Hide resolved gensim/models/poincare.py Outdated
Show resolved Hide resolved gensim/models/poincare.py Outdated
Show resolved Hide resolved gensim/models/poincare.py Outdated

koiizukag and others added some commits Jun 26, 2019

Update gensim/models/poincare.py
Co-Authored-By: Michael Penkov <m@penkov.dev>
Update gensim/models/poincare.py
Co-Authored-By: Michael Penkov <m@penkov.dev>
Update gensim/models/poincare.py
Co-Authored-By: Michael Penkov <m@penkov.dev>
Update gensim/models/poincare.py
Co-Authored-By: Michael Penkov <m@penkov.dev>
Update gensim/models/poincare.py
Co-Authored-By: Michael Penkov <m@penkov.dev>
Update gensim/models/poincare.py
Co-Authored-By: Michael Penkov <m@penkov.dev>
Update gensim/models/poincare.py
Co-Authored-By: Michael Penkov <m@penkov.dev>
@koiizukag

This comment has been minimized.

Copy link
Contributor Author

commented Jun 26, 2019

Thanks for your modification. LGTM

Update poincare.py
minor update
@koiizukag

This comment has been minimized.

Copy link
Contributor Author

commented Jun 27, 2019

@mpenkov Is there anything to be done next

@koiizukag

This comment has been minimized.

Copy link
Contributor Author

commented Jul 1, 2019

@piskvorky I think this PR is ready to merge. Please check here 🙏

@piskvorky

This comment has been minimized.

Copy link
Member

commented Jul 1, 2019

@mpenkov will be with you shortly, we have a lot going on. Thanks for your patience.

@mpenkov mpenkov merged commit e776e44 into RaRe-Technologies:develop Jul 7, 2019

3 checks passed

ci/circleci Your tests passed on CircleCI!
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@mpenkov

This comment has been minimized.

Copy link
Collaborator

commented Jul 7, 2019

Merged. @koiizukag Congratulations on your first contribution to gensim 🥇 Thank you!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.