Open gpengzhi opened 4 years ago
Merging #299 into master will increase coverage by
0.29%
. The diff coverage is86.64%
.
@@ Coverage Diff @@
## master #299 +/- ##
==========================================
+ Coverage 82.63% 82.93% +0.29%
==========================================
Files 207 215 +8
Lines 16006 17271 +1265
==========================================
+ Hits 13226 14323 +1097
- Misses 2780 2948 +168
Impacted Files | Coverage Δ | |
---|---|---|
texar/torch/utils/utils_test.py | 99.19% <100%> (+0.25%) |
:arrow_up: |
texar/torch/utils/test.py | 93.75% <100%> (+0.41%) |
:arrow_up: |
texar/torch/modules/pretrained/__init__.py | 100% <100%> (ø) |
:arrow_up: |
texar/torch/modules/encoders/__init__.py | 100% <100%> (ø) |
:arrow_up: |
texar/torch/modules/pretrained/elmo_test.py | 47.05% <47.05%> (ø) |
|
texar/torch/modules/pretrained/elmo.py | 53.84% <53.84%> (ø) |
|
texar/torch/modules/encoders/elmo_encoder_test.py | 73.33% <73.33%> (ø) |
|
texar/torch/modules/encoders/elmo_encoder.py | 74.69% <74.69%> (ø) |
|
texar/torch/utils/utils.py | 80.1% <80.64%> (+0.04%) |
:arrow_up: |
texar/torch/modules/pretrained/elmo_utils.py | 84.77% <84.77%> (ø) |
|
... and 12 more |
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 931ead9...1ad8c7d. Read the comment docs.
I agree that their implementations are a bit different and it's more reliable to use their code as is, but I don't think I would approve of merging into master at this point.
True, if I were to choose between installing allennlp
along with a plethora of dependencies, and installing this version of texar
, I would choose texar
. However, ELMo
is not the only module worth using in our package. To introduce 3k lines of code and an additional dependency is a bit too much for just one functionality. I still believe it is possible to somehow rewrite the module to use more existing parts in our package, but I agree that it would mean a lot of work and might not be worth it at this point.
Speaking of too much code, although not related to this PR, I think it's better to move the *_test.py
files out of the texar
folder, and move them to a separate tests/
folder. This way the end user doesn't have to install the test files when they do pip install texar-pytorch
. @ZhitingHu What do you think?
Add texar-styled ELMo encoder adapted from allennlp. The corresponding tokenizer will be in another PR.
Resolve some comments in #298
I checked the implementation of
ELMo
inallennlp
, It seems that they used customized LSTM such that we cannot use ourLSTM
module to implement it directly. And theHighway
module they used is different from ourHighwayWrapper
. I feel that it is better to directly use their implementations, and the correctness of the implementation is guaranteed by their unit tests. Please let me know your thought @huzecong