String.join includes an extra copy of separator at the end of the string.

[mayhewsw]

cogcomp-nlp/core-utilities/src/main/java/edu/illinois/cs/cogcomp/core/utilities/StringUtils.java

Line 60 in ce0f3a0

return sb.toString().trim();

There is a .trim() call, but this only trims whitespace. If the separator is non-whitespace (e.g. underscore, dash, period, bar, etc.), then trim doesn't work.

Also, Apache Commons has a StringUtils that works very nicely. Perhaps we want to remove this in favor of that.

[mssammon]

I agree that we should replace cogcomp's stringutils with the apache commons library. Probably a fair number of uses throughout the package, though.

[schen149]

Currently there are some feature generators in Edison that calls the StringUtils.join() to generate feature, which includes an extra copy of dash in the feature name. Example below.

cogcomp-nlp/edison/src/main/java/edu/illinois/cs/cogcomp/edison/features/helpers/FeatureNGramUtility.java

Line 55 in 7d9dad3

features.add(DiscreteFeature.create(i + ":" + StringUtils.join("-", strings)));

Switching to Apache Commons will change the feature names, and so break models that relies on Edison (PrepSRL, I think). What's the best thing to do here? @mssammon

[cogcomp-dev]

probably, requires retraining. Create a branch "requires_retrain" and make the change on this branch. Once implemented, create a new issue for prepsrl retraining and refer to it in the branch notes. presumably, it is sufficient to retrain prepsrl models and deploy, then update the dependencies on the branch; at that point, it can be merged. (You are not obligated to take that new prepsrl issue, but can if you want.)