-
Notifications
You must be signed in to change notification settings - Fork 142
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
adding transliteration models. #577
Conversation
danyaljj
commented
Oct 30, 2017
•
edited
Loading
edited
- add models for transliteration + add it to the pipeline @mayhewsw
- change viewname of relations to RELATIONS @Slash0BZ
@@ -375,7 +378,7 @@ public static BasicAnnotatorService buildPipeline(ResourceManager rm) throws IOE | |||
} | |||
if (rm.getBoolean(PipelineConfigurator.USE_RELATION)){ | |||
RelationAnnotator relationAnnotator = new RelationAnnotator(); | |||
viewGenerators.put(ViewNames.MENTION, relationAnnotator); | |||
viewGenerators.put(ViewNames.RELATION, relationAnnotator); |
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.
@Slash0BZ is this cool?
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.
I think RelationAnnotator class should be changed too, since it only adds a ViewNames.MENTION now
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.
I can do the changes. Let me know
@@ -163,7 +177,7 @@ public void addView(TextAnnotation record) throws AnnotatorException { | |||
} | |||
} | |||
} | |||
record.addView(ViewNames.MENTION, mentionView); | |||
record.addView(ViewNames.RELATION, mentionView); |
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.
@Slash0BZ how does it look now?
File modelDir = ds.getDirectory("org.cogcomp.re", "SEMEVAL", 1.1, false); | ||
modelFile = modelDir.getPath() + File.separator + "SEMEVAL" + File.separator + "SEMEVAL.lc"; | ||
lexFile = modelDir.getPath() + File.separator + "SEMEVAL" + File.separator + "SEMEVAL.lex"; | ||
} |
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.
How about here?
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.
@Slash0BZ this is ok? I think it's not working ....
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.
Did you use the same classifiers or different classifiers?
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.
My bad, I forgot it's a different classifier. The classifier for SemEval is semeval_relation_classifier
. Also you don't need the constrainedClassifier anymore, just directly use semeval_relation_classifier
and then load model and lexicon
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.
Minor changes/questions...
@@ -31,6 +31,7 @@ | |||
public static final String POS = "POS"; | |||
|
|||
public static final String MENTION = "MENTION"; | |||
public static final String RELATION = "RELATION"; |
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.
Please add a corresponding switch case in the 'getViewType()' method (https://github.com/danyaljj/illinois-cogcomp-nlp-1/blob/60b45aadcd689fbe5fc6e0af66ca0aa585c0cff7/core-utilities/src/main/java/edu/illinois/cs/cogcomp/core/datastructures/ViewNames.java#L134)
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.
Done.
@@ -175,7 +173,7 @@ public static void main(String[] args) { | |||
} | |||
|
|||
private static String annotateText(AnnotatorService finalPipeline, String text, String views, | |||
Logger logger) throws AnnotatorException { | |||
Logger logger) throws AnnotatorException { |
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.
Just curious: why is Logger a method argument, not a static class member? (the latter is more standard in CCG code)
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.
Don't remember it now, but it seems odd. Fixed it.
} | ||
|
||
/* @Test | ||
@Test | ||
public void testCleanText() throws AnnotatorException { | ||
TextAnnotation ta = DummyTextAnnotationGenerator.generateAnnotatedTextAnnotation(false, 3); | ||
annotator.getView(ta); | ||
assertEquals(true, ta.hasView(ViewNames.TRANSLITERATION)); |
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.
this is a bit minimal -- can you test that there is some expected transliteration of a word in 'ta'?
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.
Done.
throw new AnnotatorException("Missing required view SHALLOW_PARSE"); | ||
} | ||
mentionAnnotator.addView(record); | ||
if (!record.hasView(ViewNames.MENTION)) { | ||
// TODO: show error messages if the mentions are not typed. |
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.
What you can do here (outside this 'if' of course) is something like
else{
if (record.getView(ViewNames.MENTION).getConstituents().get(0).getAttribute("EntityType").equals("MENTION")){
//The model does not have type
}
}
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.
Good point.
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.
Done.
@@ -188,7 +169,7 @@ public void addView(TextAnnotation record) throws AnnotatorException { | |||
} | |||
} | |||
} | |||
record.addView(ViewNames.RELATION + "_" + type, mentionView); | |||
record.addView(ViewNames.RELATION, mentionView); |
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.
here mentionView
was initialized with ViewNames.MENTION
, and now you are adding it to ViewNames.RELATION
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.
Ah I see. I think it won't make any big problems. Will remember it ...
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.
Ok
Planning to merge on green! |