mawiesne commented on code in PR #1141:
URL: https://github.com/apache/opennlp/pull/1141#discussion_r3527136693
##########
opennlp-core/opennlp-runtime/src/main/java/opennlp/tools/sentdetect/SentenceDetectorME.java:
##########
@@ -244,77 +266,83 @@ public Span[] sentPosDetect(CharSequence s) {
}
int[] starts = ArrayMath.toIntArray(positions);
+ Span[] spans = mapPositionsToSpans(s, starts, localProbs);
- // string does not contain sentence end positions
- if (starts.length == 0) {
-
- // remove leading and trailing whitespace
- int start = 0;
- int end = s.length();
-
- while (start < s.length() && StringUtil.isWhitespace(s.charAt(start)))
- start++;
-
- while (end > 0 && StringUtil.isWhitespace(s.charAt(end - 1)))
- end--;
-
- if (end - start > 0) {
- localProbs.add(1d);
- state.sentProbs = localProbs;
- return new Span[] {new Span(start, end)};
- }
- else {
- state.sentProbs = localProbs;
- return new Span[0];
- }
- }
-
- // Convert the sentence end indexes to spans
+ // Publish for backward-compatible probs() access (last-writer-wins under
concurrency)
+ state.sentProbs = localProbs;
- boolean leftover = starts[starts.length - 1] != s.length();
- Span[] spans = new Span[leftover ? starts.length + 1 : starts.length];
+ return spans;
+ }
+ /**
+ * Maps accepted sentence-start positions to trimmed sentence {@link Span}s,
the core of the
+ * end-of-sentence position to span mapping (OPENNLP-205). Package-visible
so the mapping is
+ * directly testable without a trained model.
+ *
+ * <p>Each span runs from the previous position (or the text start) to the
next position, with
+ * Unicode {@code White_Space} trimmed from both edges. A candidate that is
whitespace-only is
+ * dropped together with its probability, so {@code probs} and the returned
spans always stay
+ * aligned; text after the last position becomes a final span with
probability {@code 1.0}.
+ * With no positions at all, the whole text is one trimmed span with
probability {@code 1.0},
+ * or no span when it is blank.</p>
+ *
+ * @param s The text the positions refer to.
+ * @param starts The accepted sentence-start positions, ascending.
+ * @param probs The probability per position; mutated (in every branch) so
it exactly mirrors
+ * the probabilities of the returned spans.
+ * @return The trimmed spans, in order, each carrying its probability via
+ * {@link Span#getProb()}.
+ */
+ static Span[] mapPositionsToSpans(CharSequence s, int[] starts, List<Double>
probs) {
+ final List<Span> spans = new ArrayList<>(starts.length + 1);
+ int sentStart = 0;
for (int si = 0; si < starts.length; si++) {
- int start;
-
- if (si == 0) {
- start = 0;
- }
- else {
- start = starts[si - 1];
- }
+ // A candidate might contain only whitespace; it is dropped together
with its probability,
+ // which keeps the spans and the probabilities aligned by construction.
+ addTrimmedSpan(spans, s, sentStart, starts[si], probs.get(si));
+ sentStart = starts[si];
+ }
+ // The text after the last position; with no positions at all this is the
whole text, which
+ // covers input without any accepted sentence end.
+ if (starts.length == 0 || starts[starts.length - 1] != s.length()) {
+ addTrimmedSpan(spans, s, sentStart, s.length(), 1d);
+ }
+ probs.clear();
+ for (Span span : spans) {
+ probs.add(span.getProb());
+ }
+ return spans.toArray(new Span[0]);
+ }
- // A span might contain only white spaces, in this case the length of
- // the span will be zero after trimming and should be ignored.
- Span span = new Span(start, starts[si]).trim(s);
- if (span.length() > 0) {
- spans[si] = span;
- }
- else {
- localProbs.remove(si);
- }
+ // Appends [start, end) as a span with the given probability attached,
unless it is
+ // whitespace-only and trims to nothing.
+ private static void addTrimmedSpan(List<Span> spans, CharSequence s, int
start, int end,
Review Comment:
For this helper method there is no need to have it `static`. Please adjust
it accordingly.
##########
opennlp-core/opennlp-runtime/src/main/java/opennlp/tools/sentdetect/SentenceDetectorME.java:
##########
@@ -244,77 +266,83 @@ public Span[] sentPosDetect(CharSequence s) {
}
int[] starts = ArrayMath.toIntArray(positions);
+ Span[] spans = mapPositionsToSpans(s, starts, localProbs);
- // string does not contain sentence end positions
- if (starts.length == 0) {
-
- // remove leading and trailing whitespace
- int start = 0;
- int end = s.length();
-
- while (start < s.length() && StringUtil.isWhitespace(s.charAt(start)))
- start++;
-
- while (end > 0 && StringUtil.isWhitespace(s.charAt(end - 1)))
- end--;
-
- if (end - start > 0) {
- localProbs.add(1d);
- state.sentProbs = localProbs;
- return new Span[] {new Span(start, end)};
- }
- else {
- state.sentProbs = localProbs;
- return new Span[0];
- }
- }
-
- // Convert the sentence end indexes to spans
+ // Publish for backward-compatible probs() access (last-writer-wins under
concurrency)
+ state.sentProbs = localProbs;
- boolean leftover = starts[starts.length - 1] != s.length();
- Span[] spans = new Span[leftover ? starts.length + 1 : starts.length];
+ return spans;
+ }
+ /**
+ * Maps accepted sentence-start positions to trimmed sentence {@link Span}s,
the core of the
+ * end-of-sentence position to span mapping (OPENNLP-205). Package-visible
so the mapping is
+ * directly testable without a trained model.
+ *
+ * <p>Each span runs from the previous position (or the text start) to the
next position, with
+ * Unicode {@code White_Space} trimmed from both edges. A candidate that is
whitespace-only is
+ * dropped together with its probability, so {@code probs} and the returned
spans always stay
+ * aligned; text after the last position becomes a final span with
probability {@code 1.0}.
+ * With no positions at all, the whole text is one trimmed span with
probability {@code 1.0},
+ * or no span when it is blank.</p>
+ *
+ * @param s The text the positions refer to.
+ * @param starts The accepted sentence-start positions, ascending.
+ * @param probs The probability per position; mutated (in every branch) so
it exactly mirrors
+ * the probabilities of the returned spans.
+ * @return The trimmed spans, in order, each carrying its probability via
+ * {@link Span#getProb()}.
+ */
+ static Span[] mapPositionsToSpans(CharSequence s, int[] starts, List<Double>
probs) {
+ final List<Span> spans = new ArrayList<>(starts.length + 1);
+ int sentStart = 0;
for (int si = 0; si < starts.length; si++) {
- int start;
-
- if (si == 0) {
- start = 0;
- }
- else {
- start = starts[si - 1];
- }
+ // A candidate might contain only whitespace; it is dropped together
with its probability,
+ // which keeps the spans and the probabilities aligned by construction.
+ addTrimmedSpan(spans, s, sentStart, starts[si], probs.get(si));
+ sentStart = starts[si];
+ }
+ // The text after the last position; with no positions at all this is the
whole text, which
+ // covers input without any accepted sentence end.
+ if (starts.length == 0 || starts[starts.length - 1] != s.length()) {
+ addTrimmedSpan(spans, s, sentStart, s.length(), 1d);
+ }
+ probs.clear();
+ for (Span span : spans) {
+ probs.add(span.getProb());
+ }
+ return spans.toArray(new Span[0]);
+ }
- // A span might contain only white spaces, in this case the length of
- // the span will be zero after trimming and should be ignored.
- Span span = new Span(start, starts[si]).trim(s);
- if (span.length() > 0) {
- spans[si] = span;
- }
- else {
- localProbs.remove(si);
- }
+ // Appends [start, end) as a span with the given probability attached,
unless it is
+ // whitespace-only and trims to nothing.
+ private static void addTrimmedSpan(List<Span> spans, CharSequence s, int
start, int end,
+ double prob) {
+ Span span = trimmedSpan(s, start, end);
+ if (span != null) {
+ spans.add(new Span(span, prob));
}
+ }
- if (leftover) {
- Span span = new Span(starts[starts.length - 1], s.length()).trim(s);
- if (span.length() > 0) {
- spans[spans.length - 1] = span;
- localProbs.add(1d);
+ // Returns [start, end) with Unicode White_Space trimmed from both edges, or
null when nothing
+ // remains. Scans by code point, matching the CharClass discipline; all
current White_Space
+ // members are BMP, but this keeps surrogate pairs at the edges intact by
construction.
+ private static Span trimmedSpan(CharSequence s, int start, int end) {
Review Comment:
For this helper method there is no need to have it `static`. Please adjust
it accordingly.
##########
opennlp-core/opennlp-runtime/src/main/java/opennlp/tools/sentdetect/SentenceDetectorME.java:
##########
@@ -244,77 +266,83 @@ public Span[] sentPosDetect(CharSequence s) {
}
int[] starts = ArrayMath.toIntArray(positions);
+ Span[] spans = mapPositionsToSpans(s, starts, localProbs);
- // string does not contain sentence end positions
- if (starts.length == 0) {
-
- // remove leading and trailing whitespace
- int start = 0;
- int end = s.length();
-
- while (start < s.length() && StringUtil.isWhitespace(s.charAt(start)))
- start++;
-
- while (end > 0 && StringUtil.isWhitespace(s.charAt(end - 1)))
- end--;
-
- if (end - start > 0) {
- localProbs.add(1d);
- state.sentProbs = localProbs;
- return new Span[] {new Span(start, end)};
- }
- else {
- state.sentProbs = localProbs;
- return new Span[0];
- }
- }
-
- // Convert the sentence end indexes to spans
+ // Publish for backward-compatible probs() access (last-writer-wins under
concurrency)
+ state.sentProbs = localProbs;
- boolean leftover = starts[starts.length - 1] != s.length();
- Span[] spans = new Span[leftover ? starts.length + 1 : starts.length];
+ return spans;
+ }
+ /**
+ * Maps accepted sentence-start positions to trimmed sentence {@link Span}s,
the core of the
+ * end-of-sentence position to span mapping (OPENNLP-205). Package-visible
so the mapping is
+ * directly testable without a trained model.
+ *
+ * <p>Each span runs from the previous position (or the text start) to the
next position, with
+ * Unicode {@code White_Space} trimmed from both edges. A candidate that is
whitespace-only is
+ * dropped together with its probability, so {@code probs} and the returned
spans always stay
+ * aligned; text after the last position becomes a final span with
probability {@code 1.0}.
+ * With no positions at all, the whole text is one trimmed span with
probability {@code 1.0},
+ * or no span when it is blank.</p>
+ *
+ * @param s The text the positions refer to.
+ * @param starts The accepted sentence-start positions, ascending.
+ * @param probs The probability per position; mutated (in every branch) so
it exactly mirrors
+ * the probabilities of the returned spans.
+ * @return The trimmed spans, in order, each carrying its probability via
+ * {@link Span#getProb()}.
+ */
+ static Span[] mapPositionsToSpans(CharSequence s, int[] starts, List<Double>
probs) {
Review Comment:
This method should be declared `private` and there is no need to have it
`static`. Please adjust it accordingly.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
