ER Classifiers switched to SparseNetworkLBP

[bhargav]

• Deployed new models and tested locally.
• Fixed #327 and #294

JointTraining was not updating weights when SparseNetworkLBP was used due to a stale reference to network. Updated the SparseNetworkLBP class to not store any values and interact with the actual class instead.

Changes in saul-core:

• Make the count variable an AtomicInteger instead of using synchronized to increment/decrement
• addInstance method has documentation and change in formatting. No functionality change.

[danyaljj]

About the changes in results of L+I: I think the existing evaluation is not quite correct.

Two scenarios:

• constrainedClassifier.test(instanceCollection): it will evaluate over all the instances given in the input.
• constrainedClassifier.test(): will collect all the instances using pathToHead; this doesn't always correspond to the instances we want/should test on. The whole point I was arguing in #294 is that we should never use pathToHead when it comes to evaluation (and it should be used only on the inference time).

I don't remember how we reported the previous results in the readme file, but I think it was using the latter. I think we should update the results after dropping pathToHead from testing.

[bhargav]

@danyaljj In the PR, deriveTestInstances method uses pathToHead to get the incident Node object. Once we have the node, I am using the getTestingInstance on the node to retrieve test candidates for the default test() method. Can you have a look at the latest iteration? I had already updated results after making those changes.

[danyaljj]

Oops. All looks good. Implementation makes sense.

[bhargav]

Updated LBJava Version and removed the SparseNetworkLBP file. Updated er-models package. But I see that some tests in POS and SRL are failing. Will investigate later.

[danyaljj]

Shoot this is a little sad ... the pre-trained models for SRL were trained on SparseNetworkLBP and need to be retrained; @kordjamshidi how hard is it to re-training predicateSenseClassifier and argumentTypeLearner and argumentXuIdentifierGivenApredicate?

[bhargav]

Yeah. This might have to wait till next week then. I updated the ER Models but other examples need to be re-trained.

@danyaljj Not sure why POS test is failing though.

[kordjamshidi]

Yes, this is what I mentioned before, we need to remove this class after the deadline.

[danyaljj]

@bhargav I think somehow the new lbjava version brings new version of Edison (which has had some changes) and causes some of the feature-extraction related stuff to fail (related to #339).

[bhargav]

@danyaljj @kordjamshidi Updated this PR. I'll have to train the SRL models again.

[kordjamshidi]

yes, it would be great to merge this one asap. @bhargav

[bhargav]

I tried running SRL training a couple of time and end up with Heap Size limit exceeded error while parsing data model. By the time 1k sentences were parsed, the process had used ~35Gb. Training examples have 37k sentences. I think we'll have to look at memory usage and fix issues there first.

[kordjamshidi]

You can look at it but I could run it by increasing the memory, using the instructions that I put here: https://github.com/IllinoisCogComp/saul/wiki/Common-Erros, maybe try this and see if it works for you. Are you sure, you really used that 35G? I think I could run the first part on my laptop also (16G).

[bhargav]

@danyaljj @kordjamshidi This is ready. The performance on SRL task was similar. It was less ( -0.5 %) on aTr but equal or better bTr, cTr, dTr, fTr.

[kordjamshidi]

ok, great.

[danyaljj]

Just a few comments. Feel free to merge after addressing them.

[danyaljj]

Small comment on the test error margin. Otherwise good to merge.

[bhargav]

Updated test margins. :)

[danyaljj]

Looks good. Merging!