Loss augmented inference using SparseNetworks #445

kordjamshidi · 2016-11-04T20:40:16Z

Also added the badge example with constraints as a test example.

-added Bhargav's fix to the intialization

kordjamshidi · 2016-11-04T21:38:28Z

@bhargav tests don't fail on my machine, I am not sure why semaphore is failing.

kordjamshidi · 2016-11-04T22:26:24Z

great, passed! please feel free to review at your earliest convenience!

danyaljj · 2016-11-05T00:14:34Z

saul-examples/src/main/scala/edu/illinois/cs/cogcomp/saulexamples/Badge/BadgeClassifiers.scala

+    override lazy val classifier = new SparseNetworkLearner()
+    override def feature = using(BadgeFeature1)
+  }
+}


could you add a little comment to each of these classifiers?

danyaljj · 2016-11-05T00:15:07Z

saul-examples/src/main/java/edu/illinois/cs/cogcomp/saulexamples/Badge/BadgeReader.java

+			br.close();
+		}catch (Exception e) {}
+	}
+}


Could you apply the autoformatter on this file?

danyaljj · 2016-11-05T00:15:31Z

saul-examples/src/main/scala/edu/illinois/cs/cogcomp/saulexamples/Badge/BadgeDataModel.scala

+      {
+        val tokens = x.split(" ")
+        tokens(1).charAt(1).toString
+      }


Drop these paranthesis?

danyaljj · 2016-11-05T00:17:14Z

saul-examples/src/main/scala/edu/illinois/cs/cogcomp/saulexamples/Badge/BadgeDataModel.scala

+        else
+          "true"
+      }
+  }


What is the purpose of doing this?

Why not re-use BadgeLabel here and say:
if(BadgeOppositLabel(x) == "true") "false" else "true"?

no specific reason, I guess the overhead is the same.

Yeah but you don't repeat the code; instead reuse it.

danyaljj · 2016-11-05T00:17:55Z

saul-examples/src/main/scala/edu/illinois/cs/cogcomp/saulexamples/Badge/BadgeClassifiers.scala

+import edu.illinois.cs.cogcomp.lbjava.learn.{ SparseNetworkLearner, SparsePerceptron }
+
+/** Created by Parisa on 9/13/16.
+  */


drop the comment?

danyaljj · 2016-11-05T00:18:09Z

saul-examples/src/main/scala/edu/illinois/cs/cogcomp/saulexamples/Badge/BadgeClassifiers.scala

+
+object BadgeClassifiers {
+  import BadgeDataModel._
+  import edu.illinois.cs.cogcomp.saul.classifier.Learnable


move this import to the top?

danyaljj · 2016-11-05T00:20:03Z

saul-examples/src/main/java/edu/illinois/cs/cogcomp/saulexamples/Badge/BadgeReader.java

+
+public class BadgeReader {
+	public List<String> badges;
+	// int currentBadge;


danyaljj · 2016-11-05T00:20:22Z

saul-core/src/main/scala/edu/illinois/cs/cogcomp/saul/classifier/infer/InitSparseNetwork.scala

@@ -33,7 +33,7 @@ object InitSparseNetwork {
              if (label >= N || iLearner.getNetwork.get(label) == null) {
                val isConjunctiveLabels = iLearner.isUsingConjunctiveLabels | iLearner.getLabelLexicon.lookupKey(label).isConjunctive
                iLearner.setConjunctiveLabels(isConjunctiveLabels)
-                val ltu: LinearThresholdUnit = iLearner.getBaseLTU
+                val ltu: LinearThresholdUnit = iLearner.getBaseLTU.clone().asInstanceOf[LinearThresholdUnit]


what is the necessity for clone()?

this bug was caught by @bhargav, it needs to create a new instance of linear threshold here each time a new label is met. This was the main bug for the SparseNetwork initialization.

danyaljj · 2016-11-05T00:21:18Z

saul-core/src/main/scala/edu/illinois/cs/cogcomp/saul/classifier/JointTrainSparseNetwork.scala

@@ -18,16 +18,16 @@ object JointTrainSparseNetwork {

  val logger: Logger = LoggerFactory.getLogger(this.getClass)
  var difference = 0
-  def apply[HEAD <: AnyRef](node: Node[HEAD], cls: List[ConstrainedClassifier[_, HEAD]], init: Boolean)(implicit headTag: ClassTag[HEAD]) = {
-    train[HEAD](node, cls, 1, init)
+  def apply[HEAD <: AnyRef](node: Node[HEAD], cls: List[ConstrainedClassifier[_, HEAD]], init: Boolean, lossAugmented: Boolean)(implicit headTag: ClassTag[HEAD]) = {


could you add a doc to this function and explain what it does as well as the parameters?

Actually here what I meant was documentation for the function.
Like:

/** * This function does blah blah ... * @param node .... * @param cls ... * .... * @param lossAugmented .... */

-fixed some indentation -one renaming

bhargav · 2016-11-06T01:01:22Z

saul-core/src/main/scala/edu/illinois/cs/cogcomp/saul/classifier/JointTrainSparseNetwork.scala

-                            ilearner.getNetwork.set(label, ltu)
+                            val ltu: LinearThresholdUnit = baseClassifier.getBaseLTU.clone().asInstanceOf[LinearThresholdUnit]
+                            ltu.initialize(baseClassifier.getNumExamples, baseClassifier.getNumFeatures)
+                            baseClassifier.getNetwork.set(label, ltu)
                            N = label + 1


This line is not required. Also N can be made a val.

bhargav · 2016-11-06T01:02:34Z

saul-core/src/main/scala/edu/illinois/cs/cogcomp/saul/classifier/JointTrainSparseNetwork.scala

-                          val ltu_actual = ilearner.getLTU(LTU_actual).asInstanceOf[LinearThresholdUnit]
-                          val ltu_predicted = ilearner.getLTU(LTU_predicted).asInstanceOf[LinearThresholdUnit]
+                          val ltu_actual = baseClassifier.getLTU(LTU_actual).asInstanceOf[LinearThresholdUnit]
+                          val ltu_predicted = baseClassifier.getLTU(LTU_predicted).asInstanceOf[LinearThresholdUnit]

                          if (ltu_actual != null)
                            ltu_actual.promote(a0, a1, 0.1)


We are promoting/demoting by a fixed update of 0.1, shouldn't we take into account the learning rate parameter. The update rule inside LinearThresholdUnit's learn function is according to the learning rate and margin thickness.

yes, this has remained here from my very first trial version. How should I pass the parameters, do you think that I just add it to the list of input parameters? Since we have two apply versions it can not have the default value for both cases as well, I guess. Isn't it a separate issue to have a consistent way for parameter setting in Saul?

The baseLTU already has all parameters to use. We can directly call the learn function to use those parameters.

val labelValues = a(3).asInstanceOf[Array[Double]] if (ltu_actual != null) { # Learn as Positive Example ltu_actual.learn(a0, a1, Array(1), labelValues) } if (ltu_predicted != null) { # Learn as a negative example ltu_predicted.learn(a0, a1, Array(0), labelValues) }

Also it might be better to rename all the variables a, a0, a1 etc for better readability.

call learn?! and what we are doing here then?

learn does not use internal prediction result?

https://github.com/IllinoisCogComp/lbjava/blob/master/lbjava/src/main/java/edu/illinois/cs/cogcomp/lbjava/learn/LinearThresholdUnit.java#L462

Learn promotes or demotes the LTU's weight vector. The third argument controls if promote should be called or demote should be called.

what about the score, s?

Looks fine if we cannot use learn. My only concern was using that having a fixed learning rate might affect performance. We can fix that separately.

bhargav · 2016-11-06T01:06:09Z

saul-core/src/main/scala/edu/illinois/cs/cogcomp/saul/classifier/JointTrainSparseNetwork.scala

  }

  @scala.annotation.tailrec
-  def train[HEAD <: AnyRef](node: Node[HEAD], cls: List[ConstrainedClassifier[_, HEAD]], it: Int, init: Boolean)(implicit headTag: ClassTag[HEAD]): Unit = {
+  def train[HEAD <: AnyRef](node: Node[HEAD], cls: List[ConstrainedClassifier[_, HEAD]], it: Int, init: Boolean, lossAugmented: Boolean = false)(implicit headTag: ClassTag[HEAD]): Unit = {
    // forall members in collection of the head (dm.t) do
    logger.info("Training iteration: " + it)


We should add an assertion here to check that the base classifiers are of the type SparseNetworkLearner. Also you can add that to the function documentation.

I guess I already had a line about it in the new documentation.

-remove redundant assignment

bhargav · 2016-11-08T21:45:33Z

saul-core/src/main/scala/edu/illinois/cs/cogcomp/saul/classifier/JointTrainSparseNetwork.scala

-                          val ltu_actual = ilearner.getLTU(LTU_actual).asInstanceOf[LinearThresholdUnit]
-                          val ltu_predicted = ilearner.getLTU(LTU_predicted).asInstanceOf[LinearThresholdUnit]
+                          val ltu_actual = baseClassifier.getLTU(LTU_actual).asInstanceOf[LinearThresholdUnit]
+                          val ltu_predicted = baseClassifier.getLTU(LTU_predicted).asInstanceOf[LinearThresholdUnit]

                          if (ltu_actual != null)
                            ltu_actual.promote(a0, a1, 0.1)


Looks fine if we cannot use learn. My only concern was using that having a fixed learning rate might affect performance. We can fix that separately.

kordjamshidi · 2016-11-08T22:40:19Z

Yes, parameter setting is a different issue that certainly should be done in a sophisticated way for join setting as well.

-smaller iterations

danyaljj · 2016-11-17T00:06:30Z

This PR looks good to me, except a few things:

Semaphore looks unhappy.
Could you keep the logger changes locally (not check in the PR), until we fix the logger issue? (very soon; I'll have it in my plans after Saul inference: moving beyond LBJava's inference #401)
Regarding adding names here and there, my suggestion is to clean them all. Git will keep track of all of your contributions, with much more detail. If we want to keep them, we probably have to make it consistent (adding them to all the files).
If there any way to verify that the loss-augmented inference is working? I just want prefer not check in something that we are not sure about its functional correctness.

kordjamshidi · 2016-11-17T00:43:05Z

see my inline comments.

kordjamshidi · 2016-11-17T00:44:22Z

This PR looks good to me, except a few things:

Semaphore looks unhappy.

Yes, I am experimenting on this and changed the path of SRL. This is strange that the same path has been used for SRL tests and SLRApps! So, changing the path there makes the tests fail! Something to work on it. To me, the whole SRLconfigurator should be removed.

Could you keep the logger changes locally (not check in the PR), until we fix the logger issue? (very soon; I'll have it in my plans after Saul inference: moving beyond LBJava's inference #401)

I did not expect right after my experimental changes today you decide to merge this since this was ready a few days ago :-). Ok.

Regarding adding names here and there, my suggestion is to clean them all. Git will keep track of all of your contributions, with much more detail. If we want to keep them, we probably have to make it consistent (adding them to all the files).

Actually, this has been always your suggestion to remove the names and I always start mentioning that I like to keep them. I am for keeping all.

If there any way to verify that the loss-augmented inference is working?

The only thing that I could do was to describe it conceptually and then test it with the Badge example, you can see the Badge example, it is one of the run options. Also, Feel free to check it algorithmically, it is a very small code.

I just want prefer not check in something that we are not sure about its functional correctness.

Me too :-).

-add loss option to SRL runnables

- added scala configurator for SRL - removed the redundant configurations from the SRL app - set the defaults to train aTR

-added results of joint training with loss-augmented inference -removed redundant property symbols

-SRL configuration back to its original -some more documentation - returned the symbol names back because of existing trained models! -

-changed back all paths and config to SRL toy

kordjamshidi · 2016-12-03T04:01:25Z

I returned back the temporary experimental changes and logger messages. Please merge if this looks fine.

kordjamshidi · 2016-12-05T02:44:23Z

@bhargav @danyaljj

kordjamshidi · 2016-12-05T02:45:35Z

@christos-c : nobody is responding here, it would be great if you could review this and merge.

bhargav · 2016-12-05T17:19:53Z

Changes look good to me. Only concern is that using loss-augmented inference does not seem to improve the performance (82.644 vs 83.673 without) -- But I saw that you mentioned in the documentation that we are performing better on some class labels.

Btw I don't seem to have permission to merge PRs. 😕

kordjamshidi · 2016-12-05T17:23:32Z

why? because you approved it probably?

kordjamshidi · 2016-12-05T17:26:20Z

We have a lack of people. I hope @christos-c can review and merge. (@bhargav, related to the loss, it has been tested on Badge example also in this PR and seems to work fine there. For improving the results with using the loss, a simple try would be working on reweighting the losses, I can do it in a different PR, and will have that as an option to tweak with it.)

christos-c · 2016-12-05T19:14:35Z

I'm on it now, will check and merge!

christos-c

The SRL part looks good, only minor typos; will merge as soon as they are fixed.

christos-c · 2016-12-05T19:20:31Z

saul-core/doc/MODELS.md

+
+    ```
+
+See [here](saul-examples/src/main/scala/edu/illinois/cs/cogcomp/saulexamples/Badge/BadgeClassifiers.scala#L43), for a working example.


This needs to be out of the code block.

christos-c · 2016-12-05T19:22:05Z

saul-core/doc/SAULLANGUAGE.md

@@ -35,8 +35,8 @@ OrgClassifier.test()

 ### Availale algorithms 
 Here is a list of available algorithms in Saul:
- - [LBJava learning algorithms](https://github.com/IllinoisCogComp/lbjava/blob/master/lbjava/doc/ALGORITHMS.md) 
- - [Weka learning algorithms](https://github.com/IllinoisCogComp/saul/blob/master/saul-core/src/main/java/edu/illinois/cs/cogcomp/saul/learn/SaulWekaWrapper.md)
+ - [LBJava learning algorithms](https://githu/IllinoisCogComp/lbjava/blob/master/lbjava/doc/ALGORITHMS.md)


The URL is wrong. Needs to be changed back to github.com

christos-c · 2016-12-05T19:22:35Z

saul-core/doc/MODELS.md

+
+    ```
+
+See [here](saul-examples/src/main/scala/edu/illinois/cs/cogcomp/saulexamples/Badge/BadgeClassifiers.scala#L43), for a working example.


The last sentence needs to be taken out of the code block.

-fixed url

kordjamshidi · 2016-12-05T21:17:48Z

@christos-c: see if this is ok now.

kordjamshidi added 5 commits November 4, 2016 13:06

-added the Badge Example

be2f7b4

-added the Badge Example Reader

b6acc34

-added Badge example with loss augmented inference

4d4d979

-added Bhargav's fix to the intialization

-format

75a071e

-fixed the test

e1a106e

kordjamshidi assigned bhargav Nov 4, 2016

-fixed the tests due to the fix in initialization

9f40b24

-test size of weights

4a578c4

learning configuration

cd57748

danyaljj reviewed Nov 5, 2016

View reviewed changes

kordjamshidi added 2 commits November 4, 2016 20:24

Merge remote-tracking branch 'upstream/master' into loss-augmented

55a808c

-added documentation

09f4aaa

-fixed some indentation -one renaming

kordjamshidi mentioned this pull request Nov 5, 2016

Next time that you want to retrain SRL ... #444

Open

-modified and documented the badge example

bca4262

bhargav reviewed Nov 6, 2016

View reviewed changes

kordjamshidi added 2 commits November 5, 2016 22:19

-assert the type

ea0198b

-remove redundant assignment

-minor

ed706a3

bhargav approved these changes Nov 8, 2016

View reviewed changes

bhargav assigned danyaljj Nov 9, 2016

-remove logger messages

e006bb9

-smaller iterations

kordjamshidi added 10 commits November 17, 2016 22:17

-jointTrain setting

ff5c9f3

-add loss option to SRL runnables

format

754d694

-fixed the test units path for SRL

92de796

- added scala configurator for SRL - removed the redundant configurations from the SRL app - set the defaults to train aTR

-format

9cf1f25

-replaced configuration parameters

5186251

-replaced configuration parameters

e1ea9a4

-added results of join training (IBT) with SRL ArgTypeClassifier

aba475b

-added results of joint training with loss-augmented inference -removed redundant property symbols

-brought the logger messages back

a65cd4e

-SRL configuration back to its original -some more documentation - returned the symbol names back because of existing trained models! -

-changed back the solver for tests

bcdba7e

-changed back the commented out join node population

dab55f1

-changed back all paths and config to SRL toy

kordjamshidi assigned christos-c and unassigned danyaljj Dec 5, 2016

christos-c requested changes Dec 5, 2016

View reviewed changes

kordjamshidi added 2 commits December 5, 2016 15:08

-fixed typos in blocking

4d8f361

-fixed url

-fixed typos in blocking

b0baf94

christos-c approved these changes Dec 5, 2016

View reviewed changes

christos-c merged commit 765f480 into CogComp:master Dec 5, 2016


		```

		See [here](saul-examples/src/main/scala/edu/illinois/cs/cogcomp/saulexamples/Badge/BadgeClassifiers.scala#L43), for a working example.

Loss augmented inference using SparseNetworks #445

Loss augmented inference using SparseNetworks #445

Conversation

kordjamshidi commented Nov 4, 2016

kordjamshidi commented Nov 4, 2016 • edited Loading

kordjamshidi commented Nov 4, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

danyaljj Nov 5, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

danyaljj Nov 14, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kordjamshidi Nov 6, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kordjamshidi commented Nov 8, 2016

danyaljj commented Nov 17, 2016 • edited by kordjamshidi Loading

kordjamshidi commented Nov 17, 2016

kordjamshidi commented Nov 17, 2016

kordjamshidi commented Dec 3, 2016

kordjamshidi commented Dec 5, 2016

kordjamshidi commented Dec 5, 2016

bhargav commented Dec 5, 2016

kordjamshidi commented Dec 5, 2016

kordjamshidi commented Dec 5, 2016 • edited Loading

christos-c commented Dec 5, 2016

christos-c left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kordjamshidi commented Dec 5, 2016 • edited Loading

kordjamshidi commented Nov 4, 2016 •

edited

Loading

danyaljj Nov 5, 2016 •

edited

Loading

danyaljj Nov 14, 2016 •

edited

Loading

kordjamshidi Nov 6, 2016 •

edited

Loading

danyaljj commented Nov 17, 2016 •

edited by kordjamshidi

Loading

kordjamshidi commented Dec 5, 2016 •

edited

Loading

kordjamshidi commented Dec 5, 2016 •

edited

Loading