krickert commented on code in PR #1141:
URL: https://github.com/apache/opennlp/pull/1141#discussion_r3528557653
##########
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:
@mawiesne On `static`: agreed, `mapPositionsToSpans`, `addTrimmedSpan`, and
`trimmedSpan` hold no instance state, so they are now instance methods
(f59954a6).
On `private`: I kept `mapPositionsToSpans` package-private and documented
why in its javadoc. It is exercised by unit tests that reach mapping branches
`sentPosDetect` cannot produce, and OpenNLP ships no mocking framework and uses
no reflection in its tests, so package visibility is the only way to keep that
coverage. Making it `private` would remove these tests:
**Lose coverage (branches unreachable through `sentPosDetect`):**
- `mapPositionsToSpansDropsAWhitespaceOnlyCandidateWithItsProbability`: the
span and its probability are dropped together; the detection loop's position
invariants never yield a whitespace-only candidate.
- `mapPositionsToSpansClearsStaleProbsInTheZeroPositionsBranch`: stale
caller entries are cleared in the zero-positions branch; production always
passes a fresh probs list.
- `mapPositionsToSpansTrimsTheFullUnicodeWhitespaceSet`: a NEL + NBSP + LS
run trimmed at a supplied mid-text position.
- `informationSeparatorsAreContentNotWhitespace`: U+001C..U+001F stay as
span-edge content; the public separator test covers candidate placement, not
edge trimming.
**Redundant with public-API tests (no coverage lost):**
- `mapPositionsToSpansHandlesNoPositions`: covered by
`inputWithoutEndOfSentenceCharactersIsOneTrimmedSpan`.
- `mapPositionsToSpansAttachesProbabilityOneInTheZeroPositionsBranch`:
assertable via `probs()` on a no-end-of-sentence input.
- `controlOnlyInputIsOneSpanOfContent`: already carries a public-API
assertion in the same test.
- `mapPositionsToSpansPreservesSupplementaryCharactersAtSpanEdges`: covered
by `supplementaryCharactersFlowThroughTheDetector`.
I'd lean on keeping it package-private, with the javadoc note explaining the
visibility (which is done). Otherwise we'd make it `private`; I drop the four
coverage tests above (and the four redundant ones) and rely on the public-API
characterization tests.
Thoughts? Is it OK as is?
--
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]
