SpanLabelView may not enforce non-overlap constraint

[mssammon]

It looks like if you call addConstituent() it sidesteps the span check: https://github.com/CogComp/cogcomp-nlp/blob/master/core-utilities/src/main/java/edu/illinois/cs/cogcomp/core/datastructures/textannotation/SpanLabelView.java#L62 This should perform a bounds check unless there is a good reason not to; otherwise, documentation should reflect the intention.

[mssammon]

Incidentally: making the Constituents field of View a TreeMap would address an inefficiency in the current code (see https://github.com/CogComp/cogcomp-nlp/blob/master/core-utilities/src/main/java/edu/illinois/cs/cogcomp/core/datastructures/textannotation/SpanLabelView.java#L65)

[ChaseDuncan]

These seem like two separate issues. Should the second issue be handled in another ticket?

Regarding the first issue, I think the fix is to simply move the bounds check from addSpanLabel(int, int, String, double) into addConstituent(Constituent) before the call to the super.addConstituent(Constituent) is made. Does that work?

[mssammon]

They are separate issues, but changing to treemap would automatically fix the duplication problem. Given that we want to order constituents for return via View.getConstituents() anyway (and we impose ordering on insertion explicitly -- see note above) it seems redundant to do both if the TreeMap change is sensible. Can you take a look and see what's involved? I assume we will need a QueryableMap class to take the place of QueryableList (currently used to hold constituents)

[mssammon]

sorry, wires crossed. Was thinking about a separate issue that I think also touches on span overlap -- duplicate constituents. Agree with your proposed solution -- moving bounds check.