two changes about joint training#296
two changes about joint training#296danyaljj wants to merge 10 commits intoCogComp:masterfrom danyaljj:mergeJointTrainingFunctions
Conversation
| typedClassifier.getCandidates(head) foreach { candidate => | ||
| typedClassifier.onClassifier.classifier match { | ||
| case _: LinearThresholdUnit => trainLinearThresholdUnitOnce[HEAD](typedClassifier, oracle, candidate) | ||
| case _: SparseNetworkLBP => trainSparseNetworkLearnerOnce[HEAD](typedClassifier, oracle, candidate) |
There was a problem hiding this comment.
We should add the default case and log a warning here.
|
Minor comment. Also this PR would conflict with the other PR on ClassifierUtils. LGTM! Assigning to parisa for any comments. |
…r the given classifier type.
|
@kordjamshidi could you comment on this? |
|
it is hard to compare since they are big chucks of moving codes, but the integration of the two is ok, if 1) you have not changed the body of jointTraining loop 2) you have tested the ER results with this change. then it is ok for me to merge. |
…pril30 # Conflicts: # saul-examples/src/main/scala/edu/illinois/cs/cogcomp/saulexamples/nlp/EntityRelation/EntityRelationApp.scala
| typedClassifier.onClassifier.classifier match { | ||
| case _: LinearThresholdUnit => trainLinearThresholdUnitOnce[HEAD](typedClassifier, oracle)(_) | ||
| case _: SparseNetworkLBP => trainSparseNetworkLearnerOnce[HEAD](typedClassifier, oracle)(_) | ||
| case _ => throw new Exception("ERROR: The joint training is not available for the classifier you have: " + |
There was a problem hiding this comment.
@kordjamshidi I changed the code (using Scala partial functions) so that it calls match-case only once. Let me know if you have any other comments on this.
BTW this is output log of ER joint-training example:
|
This is different than the results in the readme? |
|
Are the results ok for this change? should I merge this? |
|
I want to take a closer look into results. Will do it over the weekend. |
|
@danyaljj You might have to do this work afresh. There have been some changes in this and related files over the last few months. |
|
I'll wait for #380 and then send a fresh copy. |
3 participants
fyi @kordjamshidi