CogComp / saul

Saul : Declarative Learning-Based Programming
Other
64 stars 18 forks source link

Last minor modification on SRL experiments #244

Closed kordjamshidi closed 8 years ago

christos-c commented 8 years ago

@kordjamshidi There are some test failing on Semaphore. Are they passing on your computer?

Also, there are some naming conventions (capitatlization, using '_' vs camelCase, AppMultiGraph vs App), and missing documentation that would nice be there. Also, maybe you can add something about the multigraph in the README since it is unique to the SRL example?

kordjamshidi commented 8 years ago

I am not sure why semaphore is failing on my side it seems there is a problem with the pos-tagger.

kordjamshidi commented 8 years ago

I see many features are failing as well, do we get them from Curator? Maybe that is why.

christos-c commented 8 years ago

Yes, anything that's coming from Curator won't work on Semaphore, you need to switch to the pipeline but it should be just one line change.

However, there is this error:

java.lang.IllegalArgumentException: View null not found
[info]   at edu.illinois.cs.cogcomp.core.datastructures.textannotation.AbstractTextAnnotation.getView(AbstractTextAnnotation.java:123)
[info]   at edu.illinois.cs.cogcomp.edison.features.factory.ParsePath.getFeatures(ParsePath.java:55)
[info]   at edu.illinois.cs.cogcomp.edison.features.FeatureUtilities.getFeatureSet(FeatureUtilities.java:135)
[info]   at edu.illinois.cs.cogcomp.saulexamples.nlp.SemanticRoleLabeling.SRLSensors$.fexFeatureExtractor(srlSensors.scala:77)
[info]   at edu.illinois.cs.cogcomp.saulexamples.nlp.SemanticRoleLabeling.srlMultiGraph$$anonfun$26.apply(srlMultiGraph.scala:111)
[info]   at edu.illinois.cs.cogcomp.saulexamples.nlp.SemanticRoleLabeling.srlMultiGraph$$anonfun$26.apply(srlMultiGraph.scala:111)
[info]   at edu.illinois.cs.cogcomp.saul.datamodel.property.Property$class.apply(Property.scala:18)
[info]   at edu.illinois.cs.cogcomp.saul.datamodel.property.features.discrete.DiscreteProperty.apply(DiscreteProperty.scala:9)
[info]   at edu.illinois.cs.cogcomp.saul.datamodel.node.PropertySet$$anonfun$propValues$1.apply(InstanceSet.scala:58)
[info]   at scala.collection.TraversableLike$$anonfun$map$1.apply(TraversableLike.scala:245)
 ...

which seems to suggest that something else is wrong.

kordjamshidi commented 8 years ago

could you remind me how to switch to using the pipeline?

christos-c commented 8 years ago

Actually, it is already in the Configurator and it's used in populatemultiGraphwithSRLData (look at lines 36-52)

kordjamshidi commented 8 years ago

so that is not a problem then? right?

christos-c commented 8 years ago

It's not, unless you use the Curator in another place in the code.

kordjamshidi commented 8 years ago

as you see the error is in srlDataModelTest . is the Dummy generator ok?

christos-c commented 8 years ago

I'm not sure what you mean by "ok"... I've tested it before and it does exactly what it's supposed to do. But in any case 5/6 Semaphore errors are this "View null not found" type, which means that you are requesting a View that doesn't exist (the string viewName is null).

kordjamshidi commented 8 years ago

But I have not changed the code.

christos-c commented 8 years ago

I am not saying you did. Maybe it never worked on Semaphore. Either way, github will not let me merge this, unless the tests pass.

kordjamshidi commented 8 years ago

So, can't you guess why this happens? I guess you tested the features, there is the place the views are needed.

christos-c commented 8 years ago

You are the one who sent the PR! I was assigned to review (and accept) it, but you have to make sure everything works before I do. That's out github contract in every CogComp project. But if you don't have the time, then I can take a look, but it will have to wait maybe until after the hackathon.

kordjamshidi commented 8 years ago

it is not a matter of time. I thought you might see what is the reason as you tested the features. We can follow the contract!

christos-c commented 8 years ago

So the problem is that srlMultiGraph has parseViewName as an optional parameters (default set to null) and for whatever reason, it never gets set during the test.

kordjamshidi commented 8 years ago

I see thanks.

kordjamshidi commented 8 years ago

Sorry, this meant to be on my Fork.