WIP: SL integration

[kordjamshidi]

Closes #308

[kordjamshidi]

Could someone see the typing problem here: https://github.com/kordjamshidi/saul/blob/SL-Integration/saul-core/src/main/scala/edu/illinois/cs/cogcomp/saul/classifier/SL_model/SL_IOManager.scala#L18

Error:(18, 43) type mismatch;
 found   : java.util.List[edu.illinois.cs.cogcomp.saul.classifier.ConstrainedClassifier[_, HEAD]]
 required: java.util.List[edu.illinois.cs.cogcomp.saul.classifier.ConstrainedClassifier[Any,?]]
Note: edu.illinois.cs.cogcomp.saul.classifier.ConstrainedClassifier[_, HEAD] >: edu.illinois.cs.cogcomp.saul.classifier.ConstrainedClassifier[Any,?], but Java-defined trait List is invariant in type E.
You may wish to investigate a wildcard type such as `_ >: edu.illinois.cs.cogcomp.saul.classifier.ConstrainedClassifier[Any,?]`. (SLS 3.2.10)
      val ins = new Saul_SL_java_Instance(l, x)
                                          ^

I have both Java and Scala versions for this Saul_SL_java_Instance and Saul_SL_Instance and can not make none of them run/compile due to the type inconsistencies.

[kordjamshidi]

using the scala version of Saul_SL_Instance I get this runtime error :

 Error:(19, 43) type mismatch;
 found   : edu.illinois.cs.cogcomp.saul.classifier.ConstrainedClassifier[_,HEAD] => Unit
 required: edu.illinois.cs.cogcomp.saul.classifier.ConstrainedClassifier[_, HEAD] => ?
    for (c: ConstrainedClassifier[_,HEAD] <- l) {

                                          ^

Any idea what this error means in here? https://github.com/IllinoisCogComp/saul/blob/dfed404cda9bb0900c760f2560de4b9d889ee5ce/saul-core/src/main/scala/edu/illinois/cs/cogcomp/saul/classifier/SL_model/Saul_SL_Instance.scala#L19

[danyaljj]

Why not changing the signature to the following?

case class Saul_SL_Instance[HEAD<:AnyRef](l:List[ConstrainedClassifier[_,HEAD],x:HEAD) 

[kordjamshidi]

I know; this is what I had before and then I started doing aimless random combinations! I don't see the logic behind the error I receive and the signature you are suggesting does not help.

[danyaljj]

Do you get the same error when changing to what I suggested?

[kordjamshidi]

yes

[danyaljj]

What should be the output of apply function? Make the output type explicit in the code:

  def apply(implicit headTag: ClassTag[HEAD])
  = {

[danyaljj]

Like you can make it Unit or any other type:

  def apply(implicit headTag: ClassTag[HEAD]): Unit = {

[kordjamshidi]

Nope! Same error.

[danyaljj]

Could you use foreach instead of for?

l.foreach{ c: ConstrainedClassifier[_,HEAD]  => 
      val oracle: Classifier = c.onClassifier.getLabeler()
      val cands: Seq[_] = c.getCandidates(x)
      for (ci <- cands) {
        c.classifier.discreteValue(ci) //prediction result
        oracle.discreteValue(ci) // true lable
        // return a Feature values and indexs
        inputFeatures.add(c.onClassifier.getExampleArray(ci))
        factorClassifiers.add(c)
      }// yield inputFeatures
      //                    val a0 = a(0).asInstanceOf[Array[Int]]
      //                    val a1 = a(1).asInstanceOf[Array[Double]]
}

[kordjamshidi]

Yes, great, that error is gone! many thanks!

[kordjamshidi]

This is almost ready for review. There is an issue #308 with the evaluation to see the actual results using the train/test split.

[kordjamshidi]

Could someone take care of merging my lbjava PR and give me a sound version to be used here in the dependencies? my local repo for lbjava has messed up. We need it for compiling this PR. @danyaljj @christos-c

[christos-c]

That PR has still not been fixed with respect to all the formatting changes (so no one can see the changes because the log is too big for GitHub). If you send me the patch (just the code you changed) I can apply it to a fresh/latest version of LBJava. Keep in mind that we cannot test any of this because Semaphore has ran out of free builds.

[kordjamshidi]

I thought you could see the changes given the link @danyaljj sent. The problem is upgrading with upstream now.

[christos-c]

Sorry I forgot about that. Ok, I'm going to close your old PR, issue a new one from the updated upstream. Still, testing will not work, so I'm relying on you to have tested this thoroughly.

[kordjamshidi]

The old PR used to work fine if you do not introduce error s while upgrading that should be ok.

[christos-c]

I've sent a new PR. @kordjamshidi make sure the names of the variables and the description of the function makes sense. Also have you written a unit test for this? I couldn't find any.

[kordjamshidi]

This sounds great that we are this much high standard now! for two lines of code, ten lines of test units! :-). No, I have not written test unit for that small module. Does lbjava have many test units or examples that I can look at them?

[christos-c]

There is a nice test that Yiming wrote here: https://github.com/IllinoisCogComp/lbjava/blob/master/lbjava/src/test/java/edu/illinois/cs/cogcomp/lbjava/AdaGradTest.java. But yes, the problem with good software practices is that they take time (and sometimes cause bodily harm!), but it's all for a good cause!

[kordjamshidi]

I see I am the second victim, ok 👍 . and I learnt this interesting new term just now, BODILY HARM :-), "Any touching of the person of another against his will with physical force, in an intentional, hostile, and aggressive manner, or a projecting of such force against his person. People v. Moore, 50 Hun, 350, 3 N. 1. Supp. 159."

[christos-c]

I thought we were going to wait to create the small-scale unit test?

[christos-c]

@kordjamshidi because of the switch to public I cannot find the original branch of this PR which means I cannot get the SL-related code and create the toy test. Can you close this PR and create one from your public fork?

[kordjamshidi]

@christos-c : Ok, I am trying to figure out how to do it.

[kordjamshidi]

This PR copied to #341

[kordjamshidi]

@christos-c: done the copy.