krickert commented on PR #1141: URL: https://github.com/apache/opennlp/pull/1141#issuecomment-4884167027
All five points are addressed in 559ed657 (one commit, squashed). Point-by-point: **1. Whitespace change leaking into the detection loop** - kept, deliberately, and now documented + pinned. After going back and forth on this I did *not* confine `WHITESPACE` to the mapping: the whole point of the OPENNLP-1850/1852 direction is a single standards-sourced whitespace definition (UCD `White_Space`), and reintroducing `StringUtil.isWhitespace` for the loop would put two definitions inside one method. Instead the scope is stated at the `WHITESPACE` constant (it drives the skip heuristic and position placement, not just span trimming), and both loop deltas are pinned with model-driven tests: - `"This is a test.\u001CThere…"` - separator glued to the delimiter; with `useTokenEnd` the second sentence now starts after the token (`There`), without it the separator rides along as leading content - `"z.\u001Cb. x"` - the skip heuristic now merges the first delimiter into the multi-period token and only the second one is scored Per-candidate feature generation (`SDContextGenerator`) is untouched, so any candidate the model *does* evaluate sees exactly the features it was trained on. The PR description's scope claim was too narrow - the deltas are candidate-level for these control characters, and that's intended. **2. Two live whitespace definitions (`Span.trim` / `StringUtil`)** - real, and intentionally out of scope here. The `StringUtil` whitespace migration is already tracked as a scoreboard item under OPENNLP-1852; migrating `Span.trim` belongs there (it's public API used well beyond sentdetect, so it deserves its own deprecation path rather than riding along on this PR). **3. Control-only input now yields a "sentence"** - kept and pinned. It's the whole-text counterpart of the already-pinned "information separators are content" delta: if U+001C is content mid-text, `"\u001C\u001C"` can't trim to nothing. Tested through both the mapping seam and `sentPosDetect` (span `(0,2)`, prob 1.0, `probs()` aligned). **4. `probs` contract in the zero-positions branch** - fixed by removing the branch. The zero-positions case is folded into the main flow (one loop over positions + a shared tail for the remainder), and the probs list is rebuilt from the probabilities the spans themselves carry. Stale caller entries are cleared in every path, and the whole-text span now actually carries its 1.0 via `getProb()` instead of the `Span` default 0.0. Both tested, including stale-list inputs. **5. Minor cleanups** - all taken: `keptProbs` is gone (spans are the single source of truth), the trim-skip-attach triplication collapsed into one `addTrimmedSpan` helper, and the cursors (`getFirstWS`/`getFirstNonWS`/`trimmedSpan`) now scan with `codePointAt`/`codePointBefore` + `charCount` per the CharClass code-point discipline - with supplementary-plane tests (U+1D518/U+1D51E) at span edges and end-to-end through a trained model. Mapping suite went from 14 to 21 tests; full sentdetect suite (78) and the opennlp-runtime `verify` (incl. checkstyle) are green. -- 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]
