zhengzhongliang
commented
5 years ago Hi Professor, I finished most of them and have a little question about some points.
1,(Question) Please turn the manual eval that starts at line 14 in TrainDeepLearningModel into actual unit tests: There are to main methods in this block: the "EvalDeepLearningModel" method and the "ManualCheckModel". The "EvalDeepLearningModel" method just evaluates the trained model on the dev part of the spreadsheet and it is converted to unit test now. You can use the command sbt "testOnly org.clulab.reach.TestDeepLearningPolarityEngine" to test it. It should succeed if the classifier gets >0.87 dev f1 score.
The "ManualCheckModel" is for my debugging purpose. Most of the sentences are copied from the existing unit tests, with various masking schemes. Do I still need to add them? I think I need to either remove the function, or leave it there.
2, (Finished) All the models you generated and saved (and data files) such as the SavedLSTM files should be under resources/ if we use them in Reach. I can't tell where these are now. If not used in Reach, they should not be checked in* The final saved model has been moved to the resources folder and tested. Other saved models are deleted. The spreadsheet for training the model is also moved to the resources folder.
3, (Finished) Similarly, I see other big files like w2vvoc.txt that are in the top directory in Reach? These files should not be checked in. Actually these two files are not used in our model. So I just deleted them.
4, (Question) Why are there test classes called Failed_Test...? You mentioned that all tests pass now with the Hybrid method, no? And, in any case, failed tests should be marked with @ignore https://github.com/ignore, not with a change in the class name. Those failed tests are for debugging purposes when using only the deep learning model. I just copied them from the existing tests. In hybrid mode there are no failed tests. So I guess I just need to delete them.
Best regards, Zhengzhong
On Tue, Oct 15, 2019 at 10:42 AM Mihai Surdeanu notifications@github.com wrote:
@MihaiSurdeanu requested changes on this pull request.
Looks pretty good overall. A few changes:
- Please turn the manual eval that starts at line 14 in TrainDeepLearningModel into actual unit tests:
- All the models you generated and saved (and data files) such as the SavedLSTM* files should be under resources/ if we use them in Reach. I can't tell where these are now. If not used in Reach, they should not be checked in.
- Similarly, I see other big files like w2vvoc.txt that are in the top directory in Reach? These files should not be checked in.
- Why are there test classes called Failed_Test...? You mentioned that all tests pass now with the Hybrid method, no? And, in any case, failed tests should be marked with @ignore https://github.com/ignore, not with a change in the class name.
Added @enoriega https://github.com/enoriega in case you need help with these.
— You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub https://github.com/clulab/reach/pull/641?email_source=notifications&email_token=AHVRS55YWVKQ6L3CHKEBTU3QOX6IXA5CNFSM4JA7LQ6KYY3PNVWWK3TUL52HS4DFWFIHK3DMKJSXC5LFON2FEZLWNFSXPKTDN5WW2ZLOORPWSZGOCIAW27Y#pullrequestreview-302083455, or unsubscribe https://github.com/notifications/unsubscribe-auth/AHVRS52U3IWYJIJAUGDFANTQOX6IXANCNFSM4JA7LQ6A .
MihaiSurdeanu
kwalcock
Last week I found that some samples in the easy subset are not expanded very reasonably. It turns out that we were using Stanford dependency tree. I switched to the universal dependency tree in the week (for both easy and hard set), and the expanded boundary are much more reasonable. The deep learning classifier achieves the best performance so far on the unit tests (but still fails in about 16 examples). The Hybrid model passes all tests.