Saul inference: moving beyond LBJava's inference #401

danyaljj · 2016-09-25T00:34:21Z

Items to finish:

Don't use LBJava's inference and do the ILP generation based on the constrained classifier definitions directly.
Set cover should work
ER should work
SRL should work
address _atMost and _atLeast constraints do not function correctly. #147
address minor change to the way constrained classifiers are defined #217
address Extension of _forall #319
address Minor issue in SetCover #399

Results on ER (L+I):

Exactly the same numbers in the readme file:

[info] ===============================================
[info] Evaluating PerConstrainedClassifier$
[info] 
[info]  Label   Precision Recall   F1   LCount PCount
[info] ----------------------------------------------
[info] false       98.788 99.752 99.267  61178  61775
[info] true        89.088 62.362 73.367   1990   1393
[info] ----------------------------------------------
[info] Accuracy    98.574   -      -      -     63168
[info] ===============================================
[info] Evaluating OrgConstrainedClassifier$
[info] 
[info]  Label   Precision Recall   F1   LCount PCount
[info] ----------------------------------------------
[info] false       98.714 99.622 99.166  61896  62465
[info] true        66.714 36.871 47.494   1272    703
[info] ----------------------------------------------
[info] Accuracy    98.358   -      -      -     63168
[info] ===============================================
[info] Evaluating LocConstrainedClassifier$
[info] 
[info]  Label   Precision Recall   F1   LCount PCount
[info] ----------------------------------------------
[info] false       98.375 99.643 99.005  60760  61543
[info] true        86.646 58.472 69.824   2408   1625
[info] ----------------------------------------------
[info] Accuracy    98.073   -      -      -     63168
[info] ===============================================
[info] Evaluating WorksForRelationConstrainedClassifier$
[info] 
[info]  Label   Precision Recall   F1   LCount PCount
[info] ----------------------------------------------
[info] false       90.783 84.588 87.576    850    792
[info] true        25.989 38.655 31.081    119    177
[info] ----------------------------------------------
[info] Accuracy    78.947   -      -      -       969
[info] ===============================================
[info] Evaluating LivesInRelationConstrainedClassifier$
[info] 
[info]  Label   Precision Recall   F1   LCount PCount
[info] ----------------------------------------------
[info] false       76.581 94.509 84.605    692    854
[info] true        66.957 27.798 39.286    277    115
[info] ----------------------------------------------
[info] Accuracy    75.439   -      -      -       969
[info] ===============================================

kordjamshidi · 2016-11-06T19:21:43Z

Do you have any documentation that we can start reading from there?

danyaljj · 2016-11-06T19:52:02Z

Yes, see the changes.

kordjamshidi · 2016-11-06T21:00:29Z

ok, I'll do.

bhargav · 2016-11-08T21:58:48Z

saul-core/doc/SAULLANGUAGE.md

+         the inference starts from the head object. This function finds the objects of type `INPUT_TYPE` which are 
+         connected to the target object of type `HEAD_TYPE`. If we don't define `filter`, by default it returns all 
+         objects connected to `HEAD_TYPE`. The filter is useful for the `JointTraining` when we go over all 
+         global objects and generate all contained object that serve as examples for the basic classifiers involved in 


nit: objects

bhargav · 2016-11-08T22:00:18Z

saul-core/doc/SAULLANGUAGE.md

-In Saul, the constraints are defined for the assignments to class labels.
-A constraint classifiers is a classifier that predicts the class labels with regard to the specified constraints.
+In Saul, the constraints are defined for the assignments to class labels. In what follows we outine the details of operators 
+which help us define the constraints. Before jumping into the details, note that you have to have the folling import 


nit: following

bhargav · 2016-11-08T22:01:22Z

saul-core/doc/SAULLANGUAGE.md

+
+In the above definition, `on` and `is` are keywords. 
+
+Here different variations of this basic, but there are different variations to it: 


nit: didn't understand this sentence.

bhargav · 2016-11-08T22:01:55Z

saul-core/doc/SAULLANGUAGE.md

+| `ForAll`   |  For **all** the elements in the collection it applies the constraints. In other words, the constrain should hold for **all** elements of the collection.   |  `textAnnotationNode.ForAll { x: TextAnnotation => Some-Constraint-On-x }`  |    
+| `Exists`    | The constrain should hold for **at least one** element of the collection.   |  `textAnnotationNode.Exists { x: TextAnnotation => Some-Constraint-On-x }` | 
+| `AtLest(k: Int)`  |  The constrain should hold for **at least `k`** elements of the collection.  |  `textAnnotationNode.AtLeast(2) { x: TextAnnotation => Some-Constraint-On-x }` |  


nit: AtLeast(k: Int) in the first column.

bhargav · 2016-11-08T22:02:27Z

saul-core/doc/SAULLANGUAGE.md

+There are just the definitions of the operations. If you want to see real examples of the operators in actions see [the definitions of constraints for ER-example](https://github.com/IllinoisCogComp/saul/blob/master/saul-examples/src/main/scala/edu/illinois/cs/cogcomp/saulexamples/nlp/EntityRelation/EntityRelationConstraints.scala). 
+
+**Tip:** Note whenever the constrained inference is infeasible (i.e. the constraints are overlly tight), we use the default 


nit: overly

bhargav · 2016-11-08T22:03:53Z

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

@@ -19,48 +19,48 @@ object ClassifierUtils extends Logging {
    def apply[T <: AnyRef](c: (Learnable[T], Iterable[T])*) = {
      c.foreach {
        case (learner, trainInstances) =>
-          logger.info(evalSeparator)
-          logger.info("Training " + learner.getClassSimpleNameForClassifier)
+          println(evalSeparator)


Is the change from logger.info to println intentional? No strong opinion, just wanted to confirm.

I changed it because it was messing up the formatting of the output results. But now in retrospect, I think only the ones related to testing was needed. I returned back the rest.

bhargav · 2016-11-08T22:15:31Z

...ore/src/main/scala/edu/illinois/cs/cogcomp/saul/classifier/infer/ConstrainedClassifier.scala

+    val instanceIsInvolvedInConstraint = instancesInvolved.exists { set =>
+      set.exists {
+        case x: T => x == t
+        case everythingElse => false


nit: You can use _ here.

kordjamshidi · 2016-11-09T05:11:47Z

...ore/src/main/scala/edu/illinois/cs/cogcomp/saul/classifier/infer/ConstrainedClassifier.scala

+          case (singleConstraint, ins) =>
+            ins union getInstancesInvolved(singleConstraint).asInstanceOf[Set[Any]]
+        }
+      case c: AtMost[_, _] =>


Probably, I miss something here, but could you explain a bit: all these have the same body? AtMost, AtLeast, forAll...?

Good point. Merged them into one by adding an extra type.

danyaljj · 2016-11-09T22:30:02Z

Applied the comments. Let me know if you have any other comments.

kordjamshidi · 2016-11-12T04:46:59Z

...cala/edu/illinois/cs/cogcomp/saul/classifier/JoinTrainingTests/InitializeSparseNetwork.scala

+    object TestConstraintClassifier extends ConstrainedClassifier[String, String] {
+      override def subjectTo = None
+      override val solverType = OJAlgo
+      override lazy val onClassifier = TestClassifier


Maybe change the onClassifier to baseClassifier

+1 for this rename.

kordjamshidi · 2016-11-12T05:03:10Z

...linois/cs/cogcomp/saulexamples/nlp/EntityRelation/EntityRelationConstrainedClassifiers.scala

+  object OrgConstrainedClassifier extends ConstrainedClassifier[ConllRawToken, ConllRelation] {
+    override lazy val onClassifier = EntityRelationClassifiers.OrganizationClassifier
+    override def pathToHead = Some(-EntityRelationDataModel.pairTo2ndArg)
+    override def subjectTo = Some(EntityRelationConstraints.relationArgumentConstraints)


What if I want to say subjectTo = Some(worksForConstraint)? what will be the syntax? workForContatint needs an input parameter.

Another relevant comment, how can I just express true or false i.e. constant expressions as constraints. Please add these to the documentation also.

What if I want to say subjectTo = Some(worksForConstraint)? what will be the syntax? workForContatint needs an input parameter.

Regarding the first questions (as also mentioned in the documentation), the definition of the constraints starts with Node and use ForEach operator. If you want to define "worksForConstraints", instead of having it as a function:

def worksForConstraint(x: ConllRelation) = { (WorksForClassifier on x isTrue) ==> ((PersonClassifier on x.e1 isTrue) and (OrganizationClassifier on x.e2 isTrue)) }

it should be written as

def worksForConstraint = EntityRelationDataModel.pairs.ForEach { x: ConllRelation => (WorksForClassifier on x isTrue) ==> ((PersonClassifier on x.e1 isTrue) and (OrganizationClassifier on x.e2 isTrue)) }

and then you can do subjectTo = Some(worksForConstraint) ....

Another relevant comment, how can I just express true or false i.e. constant expressions as constraints. Please add these to the documentation also.

You can't (and you shouldn't) define constants.

why not mapping the case with constant True to the case with no constraints and the case with constant False to an infeasible solution message? I think it would more expressive and robust to cover all possible logical expressions.

why not mapping the case with constant True to the case with no constraints and the case with constant False to an infeasible solution message? I think it would more expressive and robust to cover all possible logical expressions.

I can almost surely guarantee that you never need to use constant True or constant False. Can you come up with an example that either of these constants are necessary?

From my view, it is a matter of completeness of the representation. And those are the basic cases that your system should not fail to address those.

If something is never needed why is it a "basic case" and "matter of completeness"?

Unless you give me a concrete example that it's needed, I won't be convinced.

Can you give me a concrete application example that a hypothesis which says h=True is useful? Why the most general hypothesish=True is discussed and mentioned at all!!
Your argument looks like this to me.

If something is never needed why is it a "basic case" and "matter of completeness?"

I missed this sentence, now I see it. I think the confusion comes from where we use first-order logic inside a different formalism that is ILP, therefore, it seems we can look at it only very practically without thinking if our constraint representation is even sound. From my view when you talk about logical expressions in any context the first expressions to think of are True and False and if I can not represent these, I am not sure what I am talking about then. We probably need to ask a third or more opinions on this. I do not have concrete examples.

kordjamshidi · 2016-11-14T18:45:09Z

For Foreach, I think the documentation is not clear enough, I can not see easily that I alway need to start writing any kind of constraint with Foreach expression, maybe you should say this above the table. Also, why this should be the case? I guess having ForAll and Foreach with two different semantics here will be confusing, from the logical expressions perspective. Unless this is technically impossible to change, I would suggest changing this.

danyaljj · 2016-11-15T09:52:42Z

For Foreach, I think the documentation is not clear enough, I can not see easily that I alway need to start writing any kind of constraint with Foreach expression, maybe you should say this above the table. Also, why this should be the case? I guess having ForAll and Foreach with two different semantics here will be confusing, from the logical expressions perspective. Unless this is technically impossible to change, I would suggest changing this.

Yes there is a conceptual difference.

Collection.ForEach{ x => .... } applies the constraint on each single instance.
Collection.ForAll{ x => .... } applies the constraint on all the instances.

For Nodes we often want the first one. In many other cases we use the second one. I will clarify this in the documentation.

Update Badge example to use new Constraint convention.

Update the Saul Inference PR

bhargav

Also I agree with @kordjamshidi 's comment about the ambiguity of ForEach and ForAll. ForEach is implemented as a quantifier that you can apply to nodes.

But ForAll is implemented as a conjunction of constraint clauses. If we plan to keep this convenience notation, we should rename to avoid confusion.

Ideally, we should be able to do node.ForAll as a quantifier.

bhargav · 2017-01-21T19:11:54Z

saul-core/doc/SAULLANGUAGE.md

+|----------|------------|---------|---|
+| `ForEach`  |  This operator works only on `Node`s. For each single instance in the node. This is often times one of the starting points for defining constraints. So if you are defining using a constrained classifier with head type `HEAD_TYPE`, we the definition of the constraint have to start with the node corresponding to this type.  |  `textAnnotationNode.ForEach { x: TextAnnotation => Some-Constraint-On-X }`   |     
+| `ForAll`   |  For **all** the elements in the collection it applies the constraints. In other words, the constrain should hold for **all** elements of the collection.   |  `textAnnotationNode.ForAll { x: TextAnnotation => Some-Constraint-On-x }`  |    


Minor typo: constrain -> constraint in this and the next 4 lines.

bhargav · 2017-01-21T19:18:26Z

...cala/edu/illinois/cs/cogcomp/saul/classifier/JoinTrainingTests/InitializeSparseNetwork.scala

+    object TestConstraintClassifier extends ConstrainedClassifier[String, String] {
+      override def subjectTo = None
+      override val solverType = OJAlgo
+      override lazy val onClassifier = TestClassifier


+1 for this rename.

bhargav · 2017-01-21T19:22:46Z

...ore/src/main/scala/edu/illinois/cs/cogcomp/saul/classifier/infer/ConstrainedClassifier.scala

+  }
+
+  /** find all the instances used in the definiton of the constraint. This is used in caching the results of inference  */
+  private def getInstancesInvolved(constraint: Constraint[_]): Set[_] = {


The getInstancesInvolved and getClassifiersInvolved methods do not depend on ConstrainedClassifer (They don't need to be here). We should add these methods to the trait/abstract class for Constraint and have them implemented there.

What abstract class?

I meant adding them to the trait for Constraint[T] here https://github.com/danyaljj/saul-1/blob/addingSaulInference/saul-core/src/main/scala/edu/illinois/cs/cogcomp/saul/classifier/infer/Constraints.scala#L114

I see; that is doable, but not sure if we would gain significant anything from that.....

danyaljj · 2017-01-22T07:34:55Z

@bhargav I think the introduction of ForAll is confusing. I can totally hide (just like the old times), and things should be simpler in that case. That sounds any good?

danyaljj · 2017-01-24T19:01:06Z

Updated the documentation to make the confusion between ForAll and ForEach clear.

bhargav · 2017-01-26T00:21:12Z

Overall the change look good. I want to do a test on the JVM memory usage after these changes. I'll do a comparative run on some of our examples. ETA: 1-2 days.

khashab2 added 29 commits September 23, 2016 16:23

basic definition of constraint datastructures.

073d9b8

equality constraints in place.

155a588

fixing some definitions and some basic expansion of the constraints.

258ffbd

small fix for tests.

ee4e5b9

fixing some issues in the implementation of constraints.

ab14102

adding unit test for inference operators.

f7442b0

more unit test for inference

af9aa78

Merge remote-tracking branch 'upstream/master' into latestSept21

defe0bf

fixed some issues. Still debugging.

13c69a3

up to at most everything works.

9d0ba78

inference tests work.

868f27b

Merge remote-tracking branch 'upstream/master' into addingSaulInference

4bff89c

minor change to setCover test.

80a5637

minor fix, again.

f486ee5

setcover test works.

f17cd92

entity-relation seem to be working.

4ccfe8d

some srl constraints in place.

6025cfe

equality constraints on two instances and two classifiers.

69ddb2c

implication rules has unit tests now.

45f7859

remove the old inference files.

6d936fd

Everything should work, except quantifier test and srl-constraint test.

5a37c25

some renaming and cleaning.

de826df

more clean up

bad08ef

more clean up.

426222c

Quantifier test should work.

4ea4b55

bring the L+I test for SRL, and no direct constraint test.

a84234a

minor clean up, again, for ER.

7340449

bring back an ER test.

4d82f99

removing some redundant comments from ConstrainedClassifier.

91bf54e

danyaljj assigned bhargav Oct 17, 2016

khashab2 added 2 commits November 6, 2016 17:05

minor change in a method attribute.

4fadd4f

drop a few obsolete tests.

17c99a1

bhargav reviewed Nov 8, 2016

View reviewed changes

kordjamshidi reviewed Nov 9, 2016

View reviewed changes

applying the comments.

5a0e6d7

khashab2 added 2 commits November 9, 2016 16:37

minor fix to type parameters.

9a4ae64

fixing some warnings related to not exhausting match.

4bce6b7

kordjamshidi reviewed Nov 12, 2016

View reviewed changes

danyaljj mentioned this pull request Nov 17, 2016

Loss augmented inference using SparseNetworks #445

Merged

Bhargav Mangipudi and others added 3 commits January 20, 2017 18:02

Merge remote-tracking branch 'upstream/master' into inference

3c2c9b9

Update Badge example to use new Constraint convention.

Fix some warnings thrown.

9aadc4d

Merge pull request #7 from bhargav/inference

a4a316c

Update the Saul Inference PR

bhargav suggested changes Jan 21, 2017

View reviewed changes

minor re-ordering of the contents.

46b4f77


		In the above definition, `on` and `is` are keywords.

		Here different variations of this basic, but there are different variations to it:

		There are just the definitions of the operations. If you want to see real examples of the operators in actions see [the definitions of constraints for ER-example](https://github.com/IllinoisCogComp/saul/blob/master/saul-examples/src/main/scala/edu/illinois/cs/cogcomp/saulexamples/nlp/EntityRelation/EntityRelationConstraints.scala).

		Tip: Note whenever the constrained inference is infeasible (i.e. the constraints are overlly tight), we use the default

Saul inference: moving beyond LBJava's inference #401

Are you sure you want to change the base?

Saul inference: moving beyond LBJava's inference #401

Conversation

danyaljj commented Sep 25, 2016 • edited Loading

Results on ER (L+I):

kordjamshidi commented Nov 6, 2016

danyaljj commented Nov 6, 2016 • edited Loading

kordjamshidi commented Nov 6, 2016

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

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 commented Nov 9, 2016

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

kordjamshidi 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kordjamshidi Nov 15, 2016 • edited Loading

Choose a reason for hiding this comment

kordjamshidi Nov 15, 2016 • edited Loading

Choose a reason for hiding this comment

kordjamshidi commented Nov 14, 2016

danyaljj commented Nov 15, 2016

bhargav 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

danyaljj commented Jan 22, 2017

danyaljj commented Jan 24, 2017

bhargav commented Jan 26, 2017

danyaljj commented Sep 25, 2016 •

edited

Loading

danyaljj commented Nov 6, 2016 •

edited

Loading

danyaljj Nov 14, 2016 •

edited

Loading

kordjamshidi Nov 14, 2016 •

edited

Loading

kordjamshidi Nov 15, 2016 •

edited

Loading

kordjamshidi Nov 15, 2016 •

edited

Loading