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

Support prediction for SRL task. #332

Merged
merged 3 commits into from
Aug 17, 2021
Merged

Support prediction for SRL task. #332

merged 3 commits into from
Aug 17, 2021

Conversation

Jiaqiang-Ruan
Copy link
Contributor

Description of changes

This PR support the prediction for SRL task. It adds user code built upon extractor framework.

Test Conducted

The code is runnable. But the performance still need to be validated on the small dataset.

Copy link
Member

@hunterhector hunterhector left a comment

Choose a reason for hiding this comment

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

I cannot fully review this without seeing work and seeing other part of the SRL system. A few comments:

  1. The code probably hide a little too much, for example, users cannot see the model class anymore
  2. If this is an real example, we should provide downloadable weights and other things for people to try.

pl = Pipeline()
pl.set_reader(reader)
pl.add(predictor)
pl.initialize()
Copy link
Member

Choose a reason for hiding this comment

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

As an example, it is better to create a writer to write out the results, or show some pack content.



reader = OntonotesReader()
saved_model = torch.load("model.pt")
Copy link
Member

Choose a reason for hiding this comment

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

Is this just the weights, or the whole model class?

text_batch=text_tensor,
text_mask=text_mask,
srl_features=srl_features)
print(output)
Copy link
Member

Choose a reason for hiding this comment

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

you probably don't want to print this in real production

@codecov
Copy link

codecov bot commented Dec 9, 2020

Codecov Report

Merging #332 (35fa6fe) into master (4e891a8) will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##           master     #332   +/-   ##
=======================================
  Coverage   81.13%   81.13%           
=======================================
  Files         209      209           
  Lines       14615    14615           
=======================================
  Hits        11858    11858           
  Misses       2757     2757           

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 4e891a8...35fa6fe. Read the comment docs.

@hunterhector hunterhector marked this pull request as ready for review August 17, 2021 16:00
@hunterhector hunterhector merged commit 9b2f3bc into asyml:master Aug 17, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants