Skip to content
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

Merged
merged 18 commits into from
Oct 24, 2022
Merged

Feature/fix gec predictor #13

merged 18 commits into from
Oct 24, 2022

Conversation

Frost45
Copy link
Collaborator

@Frost45 Frost45 commented Oct 24, 2022

In this PR, code changes have been made to the following files:

  1. datareader.py
  2. gec_predictor.py
  3. seq2labels_model.py
  4. test_seq2labels.py
  5. regression_tests/test_regression_data_predictor.py

Please review the above files.

I've added additional test cases to:

  1. test_gec_model.py
  2. 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

Copy link
Member

@desilinguist desilinguist left a 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.

Copy link
Member

@desilinguist desilinguist left a 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!

@Frost45 Frost45 changed the base branch from feature/predictor_from_gec_model to master October 24, 2022 19:37
Copy link

@ksteimel ksteimel left a 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.

gector/datareader.py Show resolved Hide resolved
gector/gec_model.py Outdated Show resolved Hide resolved
gector/gec_predictor.py Show resolved Hide resolved
gector/gec_predictor.py Show resolved Hide resolved
gector/gec_predictor.py Show resolved Hide resolved
gector/gec_predictor.py Show resolved Hide resolved
gector/gec_predictor.py Show resolved Hide resolved
@Frost45
Copy link
Collaborator Author

Frost45 commented Oct 24, 2022

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
@Frost45 Frost45 merged commit 7c5610d into master Oct 24, 2022
@Frost45 Frost45 deleted the feature/fix-gec-predictor branch October 24, 2022 22:15
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants