OPENNLP-205: Refactor the end-of-sentence position to span mapping in SentenceDetectorME#1141
Conversation
mawiesne
added
java
|
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; 1. The whitespace change leaks into the detection loop, not just the mapping ( 2. Two live whitespace definitions in the toolkit after this PR ( 3. Control-only input now yields a "sentence" ( 4. 5. Minor cleanups
|
This is all great feedback - I'll double check but this is insanely specific and really good advice. Fable it up as much as you'd like. |
|
All five points are addressed in 559ed65 (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
Per-candidate feature generation ( 2. Two live whitespace definitions ( 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, 4. 5. Minor cleanups - all taken: Mapping suite went from 14 to 21 tests; full sentdetect suite (78) and the opennlp-runtime |
| * @return The trimmed spans, in order, each carrying its probability via | ||
| * {@link Span#getProb()}. | ||
| */ | ||
| static Span[] mapPositionsToSpans(CharSequence s, int[] starts, List<Double> probs) { |
There was a problem hiding this comment.
This method should be declared private and there is no need to have it static. Please adjust it accordingly.
| } | ||
| // 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, |
There was a problem hiding this comment.
For this helper method there is no need to have it static. Please adjust it accordingly.
| // 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) { |
There was a problem hiding this comment.
For this helper method there is no need to have it static. Please adjust it accordingly.
There was a problem hiding this comment.
@mawiesne On static: agreed, mapPositionsToSpans, addTrimmedSpan, and trimmedSpan hold no instance state, so they are now instance methods (f59954a).
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 byinputWithoutEndOfSentenceCharactersIsOneTrimmedSpan.mapPositionsToSpansAttachesProbabilityOneInTheZeroPositionsBranch: assertable viaprobs()on a no-end-of-sentence input.controlOnlyInputIsOneSpanOfContent: already carries a public-API assertion in the same test.mapPositionsToSpansPreservesSupplementaryCharactersAtSpanEdges: covered bysupplementaryCharactersFlowThroughTheDetector.
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?
Pins the current behavior of SentenceDetectorME.sentPosDetect's mapping from accepted end-of-sentence positions to sentence spans, before refactoring it: exact spans and probability alignment for plain, whitespace-padded, multi-delimiter, NBSP, CRLF, whitespace-only, empty, and no-delimiter input, the free-standing-delimiter islands that map to one-character spans, and the current treatment of the next line control (U+0085), which the mapping does not recognize as whitespace. Every expectation was captured from the behavior of the current implementation, with useTokenEnd both enabled and disabled.
…lass whitespace engine Refactors the position-to-span mapping of SentenceDetectorME, open since 2011: the mapping now lives in a package-visible mapPositionsToSpans seam that is directly testable without a trained model, builds spans and probabilities in lockstep (the old pre-sized array dropped a whitespace-only candidate's probability by index, which could misalign the pairing and leave null slots), and trims with the Unicode White_Space set of CharClass.whitespace() from the normalization engine instead of StringUtil.isWhitespace. Behavior deltas, both pinned by tests: the next line control (U+0085) now counts as whitespace, so it is trimmed from span edges instead of riding along as sentence content; the U+001C..U+001F information separators, which are not Unicode White_Space, are no longer trimmed. Model feature generation (SDContextGenerator) and the break-acceptance heuristics are deliberately untouched, so existing models see exactly the features they were trained on. The characterization suite from the previous commit passes unchanged except the U+0085 expectation, updated to the corrected behavior; four new tests cover the mapping seam directly, including the whitespace-only-candidate alignment guarantee that is unreachable through the public API.
…tection-loop scope Addresses the review feedback on the span mapping refactoring in one pass, keeping the single Unicode White_Space definition (no return to StringUtil.isWhitespace anywhere). Probs contract of mapPositionsToSpans. The zero-positions branch violated the method's own javadoc twice: it never cleared entries a caller left in the probs list, and the whole-text span it returned carried the Span default probability of 0.0 instead of the 1.0 the list reported. The branch is folded into the main flow: one loop appends the trimmed span per accepted position, a shared tail appends the text after the last position (the whole text when there are no positions), and the probs list is rebuilt from the probabilities the spans themselves carry. That also removes the keptProbs bookkeeping and the three copies of the trim-skip-attach pattern, so spans and probabilities cannot drift apart by construction, in any branch. Code-point cursors. CharClass.contains takes a code point, but the whitespace cursors (getFirstWS, getFirstNonWS, trimmedSpan) fed it UTF-16 code units from charAt. Benign today (every White_Space member is in the BMP, unpaired surrogates are never members), but it relied on that property silently and broke the documented code-point discipline of the CharClass engine. All three cursors now scan with codePointAt/codePointBefore and charCount stepping, so surrogate pairs are one unit by construction. No behavior change for any current input. Detection-loop scope, stated and pinned. The White_Space set does not only trim span edges: through getFirstWS/getFirstNonWS it drives the delimiter-run skip heuristic and the placement of accepted sentence-start positions, so around U+0085 and U+001C..U+001F the candidate positions themselves can differ from pre-OPENNLP-205 releases, which is wider than a mapping-only change. This is intended: the detector keeps one whitespace definition across both stages, per the standards-sourced character class direction of OPENNLP-1850/1852. The scope is documented at the WHITESPACE constant and both loop deltas are pinned with model-driven tests ("test.\u001CThere" token placement with and without useTokenEnd; "z.\u001Cb. x" candidate skip). Per-candidate feature generation (SDContextGenerator) is untouched, so any candidate the model evaluates sees exactly the features it was trained on. Behavior delta, pinned deliberately: control-only input such as "\u001C\u001C" has no end-of-sentence character and used to trim to nothing; information separators are content under White_Space, so it is now one sentence span with probability 1.0. This is the whole-text counterpart of the already pinned informationSeparatorsAreContentNotWhitespace delta. New tests cover the stale-probs clearing for content and blank input, the attached probability of the zero-positions span, control-only input through both the mapping seam and sentPosDetect, supplementary characters (U+1D518, U+1D51E) at span edges and end-to-end through a trained model, and the two detection-loop deltas.
Addresses mawiesne's review on #1141: - mapPositionsToSpans, addTrimmedSpan, and trimmedSpan hold no instance state, so they are now instance methods rather than static. - mapPositionsToSpans stays package-private (not private): several mapping branches, the whitespace-only-candidate drop and the stale-probs reset in the zero-positions branch, are not reachable through the public sentPosDetect API, and the project ships no mocking framework and uses no reflection in tests, so package visibility is the only way to keep that branch coverage. The deliberate visibility is documented in the method javadoc. - The mapping tests call the helper on an instance instead of statically.
…alSentenceModel The Jenkins eval-tests-configurable job failed evalSentenceModel after OPENNLP-205 (d5d37dc, #1141) landed: it pins a SHA digest of every sentence SentenceDetectorME produces over the Leipzig eng_news_2010_300K corpus using the SourceForge en-sent.bin model, and OPENNLP-205 deliberately changed which characters SentenceDetectorME trims as whitespace (Unicode White_Space, not StringUtil.isWhitespace). The corpus contains at least one boundary affected by that documented delta, so the digest changed. No other test in the class touches SentenceDetectorME, and every other digest in the 138-test run was unaffected, which is consistent with an isolated, expected fallout of the OPENNLP-205 fix rather than a regression.
…nents (#1105) * OPENNLP-1865 Offset-safe, Unicode-aware input normalization in the DL components * For broader context, see epic: https://issues.apache.org/jira/browse/OPENNLP-1850 Follow-up: * OPENNLP-205: Update the stale SourceForgeModelEval golden hash for evalSentenceModel The Jenkins eval-tests-configurable job failed evalSentenceModel after OPENNLP-205 (d5d37dc, #1141) landed: it pins a SHA digest of every sentence SentenceDetectorME produces over the Leipzig eng_news_2010_300K corpus using the SourceForge en-sent.bin model, and OPENNLP-205 deliberately changed which characters SentenceDetectorME trims as whitespace (Unicode White_Space, not StringUtil.isWhitespace). The corpus contains at least one boundary affected by that documented delta, so the digest changed. No other test in the class touches SentenceDetectorME, and every other digest in the 138-test run was unaffected, which is consistent with an isolated, expected fallout of the OPENNLP-205 fix rather than a regression.
3 participants
Closes a ticket open since 2011: the mapping from accepted end-of-sentence positions to sentence spans in
SentenceDetectorME.sentPosDetectwas interwoven with the detection loop, dropped a whitespace-only candidate's probability by index from a pre-sized array (a latent span/probability misalignment), and tested whitespace withStringUtil.isWhitespace.Two commits, meant to be read in order:
useTokenEndboth enabled and disabled.mapPositionsToSpansseam that is directly testable without a trained model; spans and probabilities are built in lockstep, so they stay aligned by construction; trimming and the whitespace cursors run on the UnicodeWhite_Spaceset ofCharClass.whitespace()from the normalization engine merged with OPENNLP-1860: Unicode normalization engine — CharClass, rungs, Dimension, confusables (1a/7) #1108.Behavior deltas, both deliberate and pinned by tests: the next line control (U+0085, in Unicode
White_Spacebut missed by the old check) is now trimmed from span edges instead of riding along as sentence content, and the U+001C..U+001F information separators (not UnicodeWhite_Space) are no longer treated as whitespace.SDContextGeneratorand the break-acceptance heuristics are deliberately untouched, so existing models see exactly the features they were trained on.All sentence-detector suites pass (73 tests incl. the seven language-specific model tests), full opennlp-runtime module green.
JIRA: https://issues.apache.org/jira/browse/OPENNLP-205