rzo1 commented on PR #1141: URL: https://github.com/apache/opennlp/pull/1141#issuecomment-4883256303
Hi @krickert, I didn't have time to deeply review this yet, so here you get some Fable 5 output — take it with a grain of salt. I'll read it myself next week (human in the loop). --- **TL;DR from the model:** The refactoring itself looks sound and there are no public API breaks (nothing changed signature or visibility; `mapPositionsToSpans` is package-private). The behavioral deltas are confined to inputs containing U+0085 or U+001C–U+001F — plain ASCII/ordinary Unicode is unaffected. But two of those deltas reach further than the PR description says, plus a few smaller things: **1. The whitespace change leaks into the detection loop, not just the mapping** (`SentenceDetectorME.java:196`) `getFirstWS`/`getFirstNonWS` also drive the abbreviation-skip heuristic (line 224, `enders.get(i + 1) < fws`) and position placement (lines 241/244). Verified by simulation: for `"z.\u001Cb. x"` the old code scores the first delimiter with the model, the new code skips it — the *candidate set* changes for existing models, even though `SDContextGenerator` is untouched. For `"Hi.\u0085Next."` the accepted position shifts from 3 to 4. So "existing models see exactly the features they were trained on" is true per candidate, but *which* candidates get evaluated changes. Suggestion: either confine `WHITESPACE` to `mapPositionsToSpans`/`trimmedSpan`, or widen the stated scope and pin the skip-heuristic deltas with tests. **2. Two live whitespace definitions in the toolkit after this PR** (`SentenceDetectorME.java:315` vs `Span.java:265`) `Span.trim` still uses `StringUtil.isWhitespace` (includes U+001C–1F, excludes U+0085) — the exact opposite of the new set on those chars. Offset consumers notice: e.g. `BratDocumentParser.java:87` does `coveredIndexes.get(sentence.getStart())`, and a NEL-adjacent boundary now yields a different key. Worth either migrating `Span.trim`/`StringUtil` too (with deprecation) or explicitly noting the follow-up. **3. Control-only input now yields a "sentence"** (`SentenceDetectorME.java:282`) `"\u001C\u001C"` (no EOS chars → `starts.length == 0` branch): old code returned `new Span[0]`, new code returns `[Span(0,2)]` with prob 1.0. Not pinned by any new test — `informationSeparatorsAreContentNotWhitespace` only covers a mid-text separator with hand-fed positions. **4. `mapPositionsToSpans` breaks its own javadoc contract in the zero-positions branch** (`SentenceDetectorME.java:281`) Javadoc says `probs` is "mutated so it mirrors the returned spans", but that branch never clears the list — a non-empty input list leaves stale entries. Latent for `sentPosDetect` (fresh list each call), real for the package-visible seam. Note: the `clear()` belongs inside that branch, not at method entry (the main path reads `probs.get(si)` first). **5. Minor cleanups** - `keptProbs` is redundant — every kept `Span` already carries its prob, so the final rebuild can iterate `spans` with `getProb()`; that removes the hand-maintained parallel bookkeeping this refactor set out to eliminate (line 291). - The trim-skip-attach pattern exists in three copies (lines 281, 292, 301); also, the zero-positions branch returns a span whose `getProb()` is 0.0 while the javadoc claims every span has its probability attached. - `WHITESPACE.contains(s.charAt(pos))` feeds UTF-16 code units to a code-point API. Benign today (all Unicode `White_Space` members are BMP), but a `codePointAt` loop or a comment would match `CharClass`'s documented discipline (line 316). -- 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]
