texar-pytorch
texar-pytorch copied to clipboard
Add ELMo modules
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 in allennlp, It seems that they used customized LSTM such that we cannot use our LSTM module to implement it directly. And the Highway module they used is different from our HighwayWrapper. 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
Codecov Report
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 dataPowered 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?