-
Notifications
You must be signed in to change notification settings - Fork 2
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
Feature/fix gec predictor #13
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for figuring this out! I had some efficiency and readability suggestions.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for addressing my comments! LGTM!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for fixing these discrepancies! I'm sure this wasn't easy to track down!
I do wish stuff like appending $START was done in the dataset reader's text_to_instance method. That'd be the more allennlp-y way to handle it. However, this would break the existing gec_model.py code which is expecting to have to add $START so that doesn't really seem possible right now.
@ksteimel When we add ensemble prediction to the predictor, we can get rid of GEC_model entirely and make this change. |
…tingService/gector into feature/fix-gec-predictor
In this PR, code changes have been made to the following files:
datareader.py
gec_predictor.py
seq2labels_model.py
test_seq2labels.py
regression_tests/test_regression_data_predictor.py
Please review the above files.
I've added additional test cases to:
test_gec_model.py
test_gec_predictor.py
I've left in the comments that I added to
gec_model.py
for understanding what was happening.I have also rebased the
master
branch onto this branch.How to test:
pytest
python regression_tests/test_regression_data_predictor.py