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

Bug Fix 1779 #1787

Closed
wants to merge 1 commit into from
Closed

Bug Fix 1779 #1787

wants to merge 1 commit into from

Conversation

saroufimc1
Copy link

  • Not sure about the header.
  • Keeping the whole embedding matrix for subwords, and only creating a dictionary for the subwords using the hashing function. In the previous version, the index was incorrectly assigned in the subword dictionary.

@saroufimc1 saroufimc1 changed the title Bug Fix 1771 Bug Fix 1779 Dec 15, 2017
@menshikh-iv
Copy link
Contributor

@manneshiva @jayantj can you look into this PR?

@manneshiva
Copy link
Contributor

manneshiva commented Dec 15, 2017

@saroufimc1 You might be mixing up two issues here -- #1261 and #1779. It is best to have a different PR for each of these issues. As #1261 still needs more discussion to agree upon a solution, I suggest you only address issue #1779 here -- i.e. correct the assignment of model.wv.syn0_ngrams after trimming of unused ngrams (model.wv.syn0_ngrams.shape[0] <= self.bucket). Also, don't forget to add a test case in gensim/test/test_afsttext_wrapper.py to catch this error in future.

@saroufimc1
Copy link
Author

@manneshiva Sure, will do that.
Thanks,
Carl

@piskvorky
Copy link
Owner

Needs a better PR title and description.

Issue link belongs to description (Github's hyperlink), not the title.

@menshikh-iv
Copy link
Contributor

menshikh-iv commented Dec 25, 2017

ping @saroufimc1, what's the status of this PR?

@menshikh-iv
Copy link
Contributor

ping @saroufimc1, what's the status here?

@menshikh-iv
Copy link
Contributor

I close this PR because this looks abandoned.
@saroufimc1 feel free to re-open when you continue this.

@saroufimc1
Copy link
Author

Sorry was on vacations. Uploading my fix today. Thanks.

@saroufimc1 saroufimc1 mentioned this pull request Jan 17, 2018
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.

4 participants