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

OneHotEmbeddings has error in implementation #1190

Closed
apohllo opened this issue Oct 7, 2019 · 1 comment · Fixed by #1191
Closed

OneHotEmbeddings has error in implementation #1190

apohllo opened this issue Oct 7, 2019 · 1 comment · Fixed by #1191
Labels
bug Something isn't working

Comments

@apohllo
Copy link
Contributor

apohllo commented Oct 7, 2019

Describe the bug
The implementation of OneHotEmbeddings for fields other than text has two issues:

  • the value used in the counter is the instance of the class token, not the value of the token
  • during the computation of embedding in _add_embeddings_internal the type of the field is not taken into account

To Reproduce
Use OneHotEmbedding with field other than text and check the contents of the dictionary, which is printed out. Should include all values with count >= 3, but only includes the <unk>.

Expected behavior
The dictionary should include all values with count >= 3. During computation, the value of the specific field, not the text field should be used.

Environment (please complete the following information):

  • OS [Linux]:
  • Version [flair-0.4.3]:

Additional context
I will send a PR with the fix shortly.

@apohllo apohllo added the bug Something isn't working label Oct 7, 2019
@alanakbik
Copy link
Collaborator

@apohllo thanks for reporting this - look forward to the PR! :)

apohllo added a commit to apohllo/flair that referenced this issue Oct 7, 2019
yosipk added a commit that referenced this issue Oct 8, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants