SL-integration closes #308

kordjamshidi commented 8 years ago

This is a new PR from #247

kordjamshidi commented 8 years ago

I am not sure what has changed in the recent versions of the Lbjava with the ILPInference, when using version 24 I get this error:

Error:(6, 112) overloaded method constructor ILPInference with alternatives:
  (x$1: Any,x$2: edu.illinois.cs.cogcomp.infer.ilp.ILPSolver)edu.illinois.cs.cogcomp.lbjava.infer.ILPInference <and>
  (x$1: edu.illinois.cs.cogcomp.infer.ilp.ILPSolver,x$2: Int)edu.illinois.cs.cogcomp.lbjava.infer.ILPInference
 cannot be applied to (T, edu.illinois.cs.cogcomp.lbjava.infer.ILPSolver)
abstract class JointTemplate[T](head: T, solver: ILPSolver, norm: Normalizer = new IdentityNormalizer) extends ILPInference(head, solver) {
                                                                                                               ^

danyaljj commented 8 years ago

is it compile error?

kordjamshidi commented 8 years ago

yes.

danyaljj commented 8 years ago

it is a package name change. Remove the import (on top of the file) and add the correct one.

kordjamshidi commented 8 years ago

It seems you have two copies of some of the classes in lbjava.infer and core.infer, right?

danyaljj commented 8 years ago

I think you just need to fix the package import ...

kordjamshidi commented 8 years ago

I have it in many files, and some of them still get things from lbjava.infer

kordjamshidi commented 8 years ago

Too many package changes, I fixed the ones with I/O for Readers for pos tagger... the ILPInference issue seems to be ok now!

kordjamshidi commented 8 years ago

where is this one? PennTreebankPOSReader

danyaljj commented 8 years ago

You don't really need to ask us; you can just search inside our super-project: https://github.com/IllinoisCogComp/illinois-cogcomp-nlp/blob/master/corpusreaders/src/main/java/edu/illinois/cs/cogcomp/nlp/corpusreaders/PennTreebankPOSReader.java

kordjamshidi commented 8 years ago

You don't need to answer if too annoying!

kordjamshidi commented 8 years ago

I don't see this file in current Saul's cogcomp nlp!

danyaljj commented 8 years ago

Did this happen after changing the lbjava version? You need to see what is the latest "illinois-core-utils" and "corpus-readers" in your classpath.

danyaljj commented 8 years ago

if the issue is not resolved I can look at it tonight

kordjamshidi commented 8 years ago

No, thanks. It has been resolved.

danyaljj commented 8 years ago

@kordjamshidi is this functional/ready?

kordjamshidi commented 8 years ago

Actually, not yet, there was something wrong with the feature indexes that I could not resolve, so it does not generate the correct results for ER.

kordjamshidi commented 8 years ago

it has been a while that I've left it, but I will get back to it this weekend.

danyaljj commented 8 years ago

Could you elaborate more? Is it a very big issue? Does it make sense to merge the current version and then add the fixes later?

kordjamshidi commented 8 years ago

I don't think it is a big issue, it is related to concatenation of the feature vectors of various classifiers and making them one global vector. I do it but somewhere, something goes wrong with this module. It should be a small fix but needs a careful debug. The confusion comes from the difference between the way sparse vectors are stored in Lbjava and SL, I think. I think the code is well structured and is on the right track, I can clean it if you think we better to merge it.

danyaljj commented 8 years ago

okei then let's aim at getting this reviewed and checked then, over the next week or so.

kordjamshidi commented 8 years ago

Ok, sure. Good idea.

danyaljj commented 8 years ago

Since we changed the semaphore account, it doesn't seem to be get triggered for this PR. Will close it and open another one.

CogComp / saul

SL-integration closes #308 #341