ACRF-example-fixes #24
Conversation
removing some of the superfluous log messages that clutter up the output
| assert var1 != null : "Couldn't get label factor "+lvl1+" time "+t; | ||
| assert var2 != null : "Couldn't get label factor "+lvl2+" time "+(t+1); | ||
| protected void addInstantiatedCliques(ACRF.UnrolledGraph graph, FeatureVectorSequence fvs, LabelsAssignment lblseq) { | ||
| for (int t = 0; t < lblseq.maxTime() - 1; t++) { |
There was a problem hiding this comment.
should be looping over maxTime -- not over size()
|
Thanks! I'd be keen in merge in the change to CrossTemplate1.java, but I'm not so sure about the change to ACRF.java. The issue is that much of the time BigramTemplate is used in conjunction with UnigramTemplate for statistical reasons, because the unigram features provide a form of backoff to the bigram features. (This is probably why I hadn't noticed this issue earlier.) But if you use this version of BigramTemplate along with UnigramTemplate, then for instances with only one time slice, you would get two redundant unigram factors with different parameters. This behaviour could be confusing and maybe even lead to the model overfitting. Maybe the cleanest way to handle this is to create a template whose job is to put a unary factor only on the first labels vector in the sequence. This would fix the corner case, and also would be generally useful, as even if you are using Bigrams and Unigrams, having this behaviour is useful. Would be good to have this in a separate template so that it can be plugged in different ways. Would you have time to modify the patch accordingly? |
Bugfix for ACRF when there is only one timeslice; also fix a few errors in the example code