CogComp / saul

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

SL integration #380

Open danyaljj opened 8 years ago

danyaljj commented 8 years ago

Same as #341 and #247 Closes #308

kordjamshidi commented 8 years ago

ok, let me first update this with master! And refresh my mind with the info of two month ago!

danyaljj commented 8 years ago

Conversation moved here from #385

Interfacing the data structures of LBJava and SL makes a mess for the feature extraction part. Does anyone dare to check with me and help me debug this?

What is the issue exactly?

kordjamshidi commented 8 years ago

I am trying to update this branch, but I have problems because I've made changes to LBjava locally to be able to set weights and I see there are new changes made to LBjava and the version we use in Saul master. So, I need first send a PR to LBjava and upgrade that to be able to compile.

kordjamshidi commented 8 years ago

Lbjava's Learner has changed so much so that not a single line is in its previous place, I failed to merge with my changes, this remains to be resolved.

kordjamshidi commented 8 years ago

It would be good if you could take a look and see if you can merge these since I guess you have restructured the Learner and it is a huge file. @danyaljj

danyaljj commented 8 years ago

Do you have your LBJava modifications on a branch?

kordjamshidi commented 8 years ago

Sorry for the confusion, now just seeng an Email I see Lbjava 24 is in the repo and that is my last version :-), on July 11th. So, the problem is different than what I was thinking.

kordjamshidi commented 8 years ago

So,what are these errors that I get when merging with master: Error:(108, 29) type mismatch; found : edu.illinois.cs.cogcomp.lbjava.util.ExceptionlessInputStream required: edu.illinois.cs.cogcomp.core.datastructures.vectors.ExceptionlessInputStream forward.loadIndexWithId(in) ^

kordjamshidi commented 8 years ago

To summarize: Your help will be to merge just this with Saul/master. Lbjava version is ok. @danyaljj . Seems to be errors due to relocating packages.

kordjamshidi commented 8 years ago

It seems the inference tests do not pass?

kordjamshidi commented 8 years ago

also here I just test a simple list of constrainedClassifiers and for all instances I see this error is reported, 22:01:04 ERROR EntityRelationConstrainedClassifiers$LocConstrainedClassifier$:55 - Warning: Failed to find head could you see if this is related to the changes that recently made by @danyaljj or @bhargav . As mentioned above, it seems setcover tests also are failing. Any ideas?

kordjamshidi commented 8 years ago

It seems there are too many problems to solve with the versions and other changes, to update this PR and get to the actual problem and debugging this semantically. So, please see the issue #386 and the head not found warning which shows with our previous ER examples tests as well.

bhargav commented 8 years ago

@kordjamshidi Looking at the "Head" warning for ER example.

The Semaphore failing tests are all the new SL related ones:

[info] *** 1 TEST FAILED ***
[error] Failed tests:
[error]     edu.illinois.cs.cogcomp.saulexamples.nlp.EntityRelation.weightTest
[error] Error during tests:
[error]     edu.illinois.cs.cogcomp.saulexamples.nlp.EntityRelation.EntityRelationSLTests
[error]     edu.illinois.cs.cogcomp.saulexamples.nlp.EntityRelation.SLTest2
[error]     edu.illinois.cs.cogcomp.saulexamples.nlp.EntityRelation.EntityRelationTests

The tests fail due to:

java.io.FileNotFoundException: ../data/EntityMentionRelation/conll04_small.corp (No such file or directory)

or

[info]   java.io.FileNotFoundException: ./config/DCD.config (No such file or directory)
kordjamshidi commented 8 years ago

the path is fixed but the weightTest is due to SparseNetworkLBP change.

bhargav commented 8 years ago

I still see saul-core/src/main/scala/edu/illinois/cs/cogcomp/saul/classifier/SparseNetworkLBP.scala and saul-core/src/main/scala/edu/illinois/cs/cogcomp/saul/classifier/JoinTrainSparseNetwork.scala files in this branch. I think they were un-deleted when I merged with upstream/master.

@kordjamshidi can you delete both these files try to run the tests? There is some new code that uses SparseNetworkLBP that needs to be changed.

bhargav commented 8 years ago

@kordjamshidi Just saw your other issue about missing public features in SparseNetworkLearner. If it is easier, you can keep the SparseNetworkLBP class and use it only in the ER-SL example for now. We can decide later if we want to have both or not.

kordjamshidi commented 8 years ago

We need to use one thing consistently, since I do not create new ER classifiers for SL, I use the same classifiers of the previous ER.

kordjamshidi commented 8 years ago

I was reviewing this and I think we need to write more an more small tests to see where things go wrong. If someone can help, this would be first step: to add more to the tests. Some of the tests are failing due to the change in the data and need to be corrected too. I have added a README and tried to describe the whole structure of the code and the idea. So far my doubt was that feature indexing and making a global feature vector from all classifiers can be wrong, but it seems that part is correct, added a few tests.

kordjamshidi commented 8 years ago

there is another option also if you want: to review and merge this as it is and then work on the tests. @danyaljj @bhargav

bhargav commented 8 years ago

Will review this sometime before Friday evening. Sorry for the delay.