Skip to content

[bugfix] Bugfix embed_loader.py #128

New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

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

Conversation

LinyangHe
Copy link

Description:简要描述这次PR的内容
Bugfix embed_loader.py
Main reason: 做出这次修改的原因
Some lines in the glove file do not contain an embedded word. There're only embedding vectors in these lines. For these lines, len(line) equals to 'emb_dim', rather than 'emb_dim + 1'. Just setting 'if len(line) > 2' won't work for all the lines.

Checklist 检查下面各项是否完成

Please feel free to remove inapplicable items for your PR.

  • The PR title starts with [$CATEGORY] (例如[bugfix]修复bug,[new]添加新功能,[test]修改测试,[rm]删除旧代码)
  • Changes are complete (i.e. I finished coding on this PR) 修改完成才提PR
  • All changes have test coverage 修改的部分顺利通过测试。对于fastnlp/fastnlp/的修改,测试代码必须提供在fastnlp/test/
  • Code is well-documented 注释写好,API文档会从注释中抽取
  • To the my best knowledge, examples are either not affected by this change, or have been fixed to be compatible with this change 修改导致例子或tutorial有变化,请找核心开发人员

Changes: 逐项描述修改的内容

  • modify 'if len(line) > 2' to 'if len(line) == emb_dim + 1'
  • add parameter 'emb_dim' in function '_load_pretrain()' and '_load_glove()'

Mention: 找人review你的PR
@xuyige

@codecov-io
Copy link

Codecov Report

Merging #128 into master will not change coverage.
The diff coverage is 0%.

Impacted file tree graph

@@          Coverage Diff          @@
##           master   #128   +/-   ##
=====================================
  Coverage      68%    68%           
=====================================
  Files          90     90           
  Lines        6286   6286           
=====================================
  Hits         4275   4275           
  Misses       2011   2011
Impacted Files Coverage Δ
fastNLP/io/embed_loader.py 55.73% <0%> (ø) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 3fa95b6...31c564c. Read the comment docs.

@xpqiu xpqiu requested a review from choosewhatulike January 18, 2019 07:37
@choosewhatulike
Copy link
Member

Could you please provide a small piece of glove that cause the bug as an example at here? As we haven't met such issue when using Glove. Thanks.

# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants