[
https://issues.apache.org/jira/browse/OPENNLP-1989?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=18120854#comment-18120854
]
ASF GitHub Bot commented on OPENNLP-1989:
-----------------------------------------
rzo1 commented on code in PR #1321:
URL: https://github.com/apache/opennlp/pull/1321#discussion_r4136825586
##########
opennlp-core/opennlp-ml/opennlp-ml-commons/src/main/java/opennlp/tools/ml/model/RealValueFileEventStream.java:
##########
@@ -125,21 +131,41 @@ public static float[] parseContexts(String[] contexts) {
return values;
}
+ /**
+ * Parses one event line. Fields are separated by runs of space, tab,
carriage return,
+ * line feed and form feed, as in {@link FileEventStream}; the first field
is the outcome,
+ * the rest are contexts parsed by {@link #parseContexts(String[])}.
+ *
+ * @param line The event line. Must not be {@code null}.
+ * @return The event; a line with only an outcome gives an event without
contexts.
+ * @throws IllegalArgumentException Thrown if {@code line} is {@code null}
or a value is negative,
+ * NaN or infinite.
+ * @throws InvalidFormatException Thrown if {@code line} is blank.
+ */
+ public static Event parseEvent(String line) throws InvalidFormatException {
Review Comment:
The vertical tab was a separator here and in `RealBasicEventStream`
(`split("\\s+")`) and isn't one anymore. That's only mentioned in a test
comment. Please add it to the description next to the outcome-only change.
##########
opennlp-core/opennlp-ml/opennlp-ml-commons/src/test/java/opennlp/tools/ml/AbstractEventStreamTest.java:
##########
@@ -60,6 +60,14 @@ public abstract class AbstractEventStreamTest {
protected static final String EVENTS_INVALID_NEGATIVE =
"other wc=ic=-1.0 w&c=he,ic=-2.0 n1wc=lc=-3.0 n1w&c=belongs,lc=-4.0
n2wc=lc=-5.0\n";
+ /**
+ * Characters that stay inside an event field: Unicode whitespace outside
the delimiter set,
+ * the vertical tab, which {@code split("\\s+")} used to treat as a
separator, a file separator,
Review Comment:
"which `split("\\s+")` used to treat as a separator" is history. Say what
the test checks.
##########
opennlp-docs/src/docbkx/machine-learning.xml:
##########
@@ -86,6 +86,22 @@ under the License.
component which uses this functionality.
</para>
<para>
+ <code>FileEventStream</code> reads events from a text file, one
event per line, and
+ <code>RealValueFileEventStream</code> and
<code>RealBasicEventStream</code> read events
+ with real-valued features. The first field of a line is the
outcome and each further field
+ is a context, optionally followed by <code>=</code> and its
value after the last
+ <code>=</code>. Space, tab, carriage return, line feed and form
feed separate fields; other
+ characters, including no-break spaces, stay inside feature
names. A line with only an
+ outcome is an event without contexts, and a blank line is an
error. Since 3.0, an outcome-only line
Review Comment:
Much longer than its neighbours, please rewrap.
##########
opennlp-core/opennlp-ml/opennlp-ml-commons/src/test/java/opennlp/tools/ml/model/RealValueFileEventStreamTest.java:
##########
@@ -92,4 +81,193 @@ void testReset() throws IOException {
} catch (UnsupportedOperationException expected) {
}
}
+
+ @Test
+ void testReadSplitsContextsOnWhitespaceRuns() throws IOException {
+ String input = "other wc=ic=1.0\t\tw&c=he,ic=2.0 n1wc=lc=3.0 \t \n"
+ + "other wc=lc=1.0 w&c=belongs,lc=2.0\u00A0p1wc=ic=3.0\n"
+ + "other wc=lc=1.0 w&c=to,lc=2.0 \n";
+ try (ObjectStream<Event> eventStream = createEventStream(input)) {
+ Event e = eventStream.read();
+ Assertions.assertArrayEquals(
+ new String[] {"wc=ic", "w&c=he,ic", "n1wc=lc"}, e.getContext());
+ Assertions.assertArrayEquals(new float[] {1.0f, 2.0f, 3.0f},
e.getValues());
+ e = eventStream.read();
+ Assertions.assertArrayEquals(
+ new String[] {"wc=lc", "w&c=belongs,lc=2.0\u00A0p1wc=ic"},
e.getContext());
+ Assertions.assertArrayEquals(new float[] {1.0f, 3.0f}, e.getValues());
+ // repeated runs produce no empty predicate
+ e = eventStream.read();
+ Assertions.assertArrayEquals(
+ new String[] {"wc=lc", "w&c=to,lc"}, e.getContext());
+ Assertions.assertArrayEquals(new float[] {1.0f, 2.0f}, e.getValues());
+ Assertions.assertNull(eventStream.read());
+ }
+ }
+
+ private static Stream<Arguments> fieldSeparators() {
+ return Stream.of(
+ // the fixed event separators, including runs and leading separators
+ Arguments.of("other\twc=ic=1.0", "other", new String[] {"wc=ic"}),
+ Arguments.of("other \t wc=ic=1.0", "other", new String[] {"wc=ic"}),
+ Arguments.of("other\fwc=ic=1.0", "other", new String[] {"wc=ic"}),
+ Arguments.of(" other wc=ic=1.0", "other", new String[] {"wc=ic"}),
+ Arguments.of("\tother wc=ic=1.0", "other", new String[] {"wc=ic"}),
+ Arguments.of("other wc=ic=1.0\r", "other", new String[] {"wc=ic"}),
+ // non-delimiter characters remain part of the outcome
+ Arguments.of("other\u00A0wc=ic=1.0", "other\u00A0wc=ic=1.0", new
String[0]),
+ Arguments.of("other\u0085wc=ic=1.0", "other\u0085wc=ic=1.0", new
String[0]),
+ Arguments.of("other\u2028wc=ic=1.0", "other\u2028wc=ic=1.0", new
String[0]),
+ Arguments.of("other\u3000wc=ic=1.0", "other\u3000wc=ic=1.0", new
String[0]),
+ // file separator and zero width space are not delimiters either
+ Arguments.of("other\u001Cwc=ic=1.0", "other\u001Cwc=ic=1.0", new
String[0]),
+ Arguments.of("other\u200Bwc=ic=1.0", "other\u200Bwc=ic=1.0", new
String[0]),
+ Arguments.of("other wc\u001C=ic=1.0", "other", new String[]
{"wc\u001C=ic"}),
+ // a supplementary character is one character of a field
+ Arguments.of("\uD83D\uDE00 w=\uD83D\uDE00=1.0", "\uD83D\uDE00", new
String[] {"w=\uD83D\uDE00"}));
+ }
+
+ @ParameterizedTest
+ @MethodSource("fieldSeparators")
+ void testFieldsUseEventDelimiters(String line, String outcome, String[]
contexts)
+ throws IOException {
+ Event e = RealValueFileEventStream.parseEvent(line);
+ Assertions.assertEquals(outcome, e.getOutcome());
+ Assertions.assertArrayEquals(contexts, e.getContext());
+ }
+
+ @Test
+ void testParseEventRejectsNull() {
+ IllegalArgumentException e =
Assertions.assertThrows(IllegalArgumentException.class,
+ () -> RealValueFileEventStream.parseEvent(null));
+ Assertions.assertEquals("line must not be null", e.getMessage());
+ }
+
+ @ParameterizedTest
+ @ValueSource(strings = {"other", "other ", " other\t"})
+ void testOutcomeOnlyLineHasNoContexts(String line) throws IOException {
+ Event e = RealValueFileEventStream.parseEvent(line);
+ Assertions.assertEquals("other", e.getOutcome());
+ Assertions.assertEquals(0, e.getContext().length);
+ Assertions.assertNull(e.getValues());
+ }
+
+ @ParameterizedTest
+ @ValueSource(strings = {"", " ", "\t", "\f", " \t\r\n\f", "\r"})
+ void testLineWithoutOutcomeIsRejected(String line) {
+ InvalidFormatException e =
Assertions.assertThrows(InvalidFormatException.class,
+ () -> RealValueFileEventStream.parseEvent(line));
+ Assertions.assertEquals("An event line must start with an outcome: \"" +
line + "\"", e.getMessage());
+ }
+
+ @ParameterizedTest
+ @ValueSource(strings = {"org/start", "a=b", "x;y", "B-ORG", "\u00E9tat",
"\uD83D\uDE00"})
+ void testOutcomeMayContainAnyNonWhitespace(String outcome) throws
IOException {
+ Event e = RealValueFileEventStream.parseEvent(outcome + " wc=ic=1.0");
+ Assertions.assertEquals(outcome, e.getOutcome());
+ Assertions.assertArrayEquals(new String[] {"wc=ic"}, e.getContext());
+ }
+
+ private static Stream<Arguments> contextValues() {
+ return Stream.of(
+ Arguments.of("w&c=he,ic=2.0", "w&c=he,ic", 2.0f),
+ Arguments.of("a=b=c=3.5", "a=b=c", 3.5f),
+ Arguments.of("wc=ic=1e2", "wc=ic", 100f),
+ Arguments.of("wc=ic=1E-2", "wc=ic", 0.01f),
+ Arguments.of("wc=ic=.5", "wc=ic", 0.5f),
+ Arguments.of("wc=ic=+2", "wc=ic", 2f),
+ Arguments.of("wc=ic=0", "wc=ic", 0f));
+ }
+
+ /** The value follows the last {@code =}, so a context name may contain the
character. */
+ @ParameterizedTest
+ @MethodSource("contextValues")
+ void testContextValueFollowsTheLastEqualsSign(String context, String name,
float value)
+ throws IOException {
+ Event e = RealValueFileEventStream.parseEvent("other " + context);
+ Assertions.assertArrayEquals(new String[] {name}, e.getContext());
+ Assertions.assertArrayEquals(new float[] {value}, e.getValues());
+ }
+
+ @ParameterizedTest
+ // no equals sign, an empty value after it, an equals sign in first
position, text or a comma after it
+ @ValueSource(strings = {"wc", "wc=", "=5", "wc=abc", "wc=1,5",
"wc=ic=1.0.0"})
+ void testContextWithoutANumberIsKeptWholeAndCountsOnce(String context)
throws IOException {
+ Event e = RealValueFileEventStream.parseEvent("other " + context);
+ Assertions.assertArrayEquals(new String[] {context}, e.getContext());
+ Assertions.assertNull(e.getValues());
+ }
+
+ @Test
+ void testContextsWithAndWithoutValuesMix() throws IOException {
+ Event e = RealValueFileEventStream.parseEvent("other wc=ic=2.0 wc
n1wc=lc=0.5");
+ Assertions.assertArrayEquals(new String[] {"wc=ic", "wc", "n1wc=lc"},
e.getContext());
+ Assertions.assertArrayEquals(new float[] {2.0f, 1.0f, 0.5f},
e.getValues());
+ }
+
+ @ParameterizedTest
+ @ValueSource(strings = {"wc=ic=-1", "wc=ic=-0.5", "wc=ic=-1e2",
"wc=ic=-1E-2"})
+ void testNegativeValueIsRejectedWithTheContextNamed(String context) {
+ IllegalArgumentException e =
Assertions.assertThrows(IllegalArgumentException.class,
+ () -> RealValueFileEventStream.parseEvent("other " + context));
+ Assertions.assertEquals("Negative values are not allowed: " + context,
e.getMessage());
+ }
+
+ /** {@code Float.parseFloat} accepts these, but they are no usable feature
values. */
+ @ParameterizedTest
+ @ValueSource(strings = {"wc=ic=NaN", "wc=ic=Infinity", "wc=ic=-Infinity"})
+ void testNonFiniteValueIsRejectedWithTheContextNamed(String context) {
+ IllegalArgumentException e =
Assertions.assertThrows(IllegalArgumentException.class,
+ () -> RealValueFileEventStream.parseEvent("other " + context));
+ Assertions.assertEquals("Values must be finite: " + context,
e.getMessage());
+ }
+
+ @ParameterizedTest
+ @ValueSource(strings = {"\n", "\r\n", "\r"})
+ void testReadAcceptsEveryLineTerminator(String terminator) throws
IOException {
+ String input = "other wc=ic=1.0" + terminator + "other wc=lc=2.0" +
terminator;
+ try (ObjectStream<Event> eventStream = createEventStream(input)) {
+ Assertions.assertArrayEquals(new String[] {"wc=ic"},
eventStream.read().getContext());
+ Assertions.assertArrayEquals(new String[] {"wc=lc"},
eventStream.read().getContext());
+ Assertions.assertNull(eventStream.read());
+ }
+ }
+
+ @Test
+ void testReadRejectsBlankLine() throws IOException {
+ try (ObjectStream<Event> eventStream = createEventStream("other
wc=ic=1.0\n\nother wc=lc=1.0\n")) {
+ Assertions.assertEquals("other", eventStream.read().getOutcome());
+ Assertions.assertThrows(InvalidFormatException.class, eventStream::read);
+ }
+ }
+
+ /** See {@link #NON_DELIMITER_CHARS}: only the vertical tab row pins a
change from 2.x. */
Review Comment:
"pins a change from 2.x" is history. Say what the test checks.
> Regex removal (5b/10): Make event parsing deterministic and preserve feature
> identities
> ---------------------------------------------------------------------------------------
>
> Key: OPENNLP-1989
> URL: https://issues.apache.org/jira/browse/OPENNLP-1989
> Project: OpenNLP
> Issue Type: Improvement
> Reporter: Kristian Rickert
> Priority: Major
> Time Spent: 10m
> Remaining Estimate: 0h
>
> Correct parsing in {{RealValueFileEventStream}}, {{RealBasicEventStream}},
> and {{SimpleEventStreamBuilder}} using explicit, documented delimiters
> independent of global tokenizer settings. Preserve Unicode characters within
> feature names, eliminate accidental empty features, and prevent outcome-only
> events from prematurely ending a stream. Report malformed records clearly and
> document the shared parsing API. Test exact outcomes, feature arrays, values,
> and continued reading after outcome-only events.
--
This message was sent by Atlassian Jira
(v8.20.10#820010)
