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

Allennlp Predictor class that mimics gec_model #6

Merged
merged 25 commits into from
Oct 24, 2022

Conversation

ksteimel
Copy link

Sorry for the large PR. These changes introduce a gec_predictor class that is able to mimic the behavior of gec_model but in a way that follows the allennlp api.
This does handle multi-iteration prediction. However, gec_model had an option for ensemble prediction and that is currently not handled here.
I have also created a model archive that can be used with this new predictor. You can see the config file for this in test_fixtures/roberta_model. The weights file is downloaded the appropriate location and shared between the predictor and gec_model.. I can tar the folder up and then add it to a public s3 bucket after this PR is approved.

…y gec_model,

get_token_action removed and placed in model.
There's a bug in here somewhere though because the output isn't exactly the same. I suspect there's an off by one error happening when the indices for edit operations are generated.
… errors are identified.

Fixing offsets in light of addition of START_TOKEN to front.
Everything works now except that the text is getting lowercased somewhere.
…offsets when creating the target (corrected sentence).

Copying get_token_action method almost verbatim instead of using something more clever.
…. This is identical to the output for gec_model!
@ksteimel
Copy link
Author

Sorry! I forgot to add Nitin and Damien to the list of reviewers somehow.

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.

I am not that familiar with this codebase so I am going to approve based on what I see and let @Frost45 and @damien2012eng do a more careful review.

@damien2012eng
Copy link
Collaborator

Great work, @ksteimel !
Just wondering, the intension of this PR is not to replace the gec_model, but to introduce gec_predictor and the config file. Am I understanding it correctly?

@ksteimel ksteimel requested a review from mulhod October 5, 2022 14:58
Copy link
Collaborator

@Frost45 Frost45 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is amazing work @ksteimel! Thank you for taking this on! Just a few minor comments.

gector/gec_model.py Outdated Show resolved Hide resolved
gector/gec_predictor.py Show resolved Hide resolved
gector/gec_predictor.py Outdated Show resolved Hide resolved
gector/gec_predictor.py Outdated Show resolved Hide resolved
gector/gec_predictor.py Show resolved Hide resolved
gector/seq2labels_model.py Outdated Show resolved Hide resolved
gector/seq2labels_model.py Show resolved Hide resolved
tests/test_gec_predictor.py Outdated Show resolved Hide resolved
tests/test_gec_predictor.py Show resolved Hide resolved
tests/test_gec_predictor.py Outdated Show resolved Hide resolved
@Frost45 Frost45 self-requested a review October 6, 2022 18:27
@damien2012eng damien2012eng self-requested a review October 11, 2022 18:54
@ksteimel ksteimel merged commit c937981 into master Oct 24, 2022
@Frost45 Frost45 deleted the feature/predictor_from_gec_model branch October 25, 2022 12:23
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.

4 participants