krickert commented on code in PR #1105: URL: https://github.com/apache/opennlp/pull/1105#discussion_r3523603192
########## opennlp-api/src/main/java/opennlp/tools/namefind/OffsetMappingNameFinder.java: ########## @@ -0,0 +1,44 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one or more + * contributor license agreements. See the NOTICE file distributed with + * this work for additional information regarding copyright ownership. + * The ASF licenses this file to You under the Apache License, Version 2.0 + * (the "License"); you may not use this file except in compliance with + * the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ +package opennlp.tools.namefind; + +import opennlp.tools.util.Span; + +/** + * A {@link TokenNameFinder} that can additionally report detected spans in the character coordinates + * of the original input, mapping back through any text normalization applied before detection. + * + * <p>An implementation that normalizes input before detection (for example an ONNX model that folds + * Unicode whitespace or dashes) returns spans from {@link #find(String[])} in the coordinates of the + * normalized text, which no longer line up with the caller's input when a fold changes the length. + * {@link #findInOriginal(String[])} maps those spans back to original-input coordinates. This is a + * separate capability interface rather than a method on {@link TokenNameFinder} because the classic + * contract reports token-index spans, for which an original-character mapping is not meaningful; an + * interface-typed caller tests for the capability ({@code finder instanceof OffsetMappingNameFinder}) + * instead of depending on a concrete implementation.</p> + */ +public interface OffsetMappingNameFinder extends TokenNameFinder { + + /** + * Finds names and returns their {@link Span spans} in the character coordinates of the original + * input, regardless of any normalization applied before detection. + * + * @param tokens The tokens to search. + * @return The detected spans, in original-input character coordinates. + */ + Span[] findInOriginal(String[] tokens); Review Comment: Added `@throws IllegalArgumentException` to the interface contract (impl already validates via `locate` → `requireNonNullArg` + null-token check) ########## opennlp-core/opennlp-ml/opennlp-dl/src/main/java/opennlp/dl/namefinder/NameFinderDL.java: ########## @@ -153,69 +163,173 @@ public NameFinderDL(File model, File vocabulary, Map<Integer, String> ids2Labels this.includeTokenTypeIds = inferenceOptions.isIncludeTokenTypeIds(); this.documentSplitSize = inferenceOptions.getDocumentSplitSize(); this.splitOverlapSize = inferenceOptions.getSplitOverlapSize(); + this.normalizeWhitespace = inferenceOptions.isNormalizeWhitespace(); + this.normalizeDashes = inferenceOptions.isNormalizeDashes(); this.sentenceDetector = sentenceDetector; } private static InferenceOptions validateConstructorArguments( final InferenceOptions inferenceOptions, final Map<Integer, String> ids2Labels, final SentenceDetector sentenceDetector) { - Objects.requireNonNull(ids2Labels, "ids2Labels"); - Objects.requireNonNull(sentenceDetector, "sentenceDetector"); + requireNonNullArg(inferenceOptions, "inferenceOptions"); + requireNonNullArg(ids2Labels, "ids2Labels"); + requireNonNullArg(sentenceDetector, "sentenceDetector"); return inferenceOptions; } + /** * {@inheritDoc} * - * <p>This method joins the provided tokens with spaces, sentence-splits the joined text, - * runs each sentence through the ONNX token-classification model, decodes BIO labels into - * {@link Span spans}, and resolves those spans back to character offsets in the joined text.</p> + * <p>Joins the provided tokens with spaces, sentence-splits the joined text, runs each sentence + * through the ONNX token-classification model, decodes BIO labels into {@link Span spans}, and + * resolves those spans to character offsets in the joined text <em>after</em> any optional input + * normalization.</p> + * + * <p>Note: this returns correct original offsets in every case except one. Whitespace folding is + * length-preserving, so it never moves offsets. Only dash folding can change the input length, and + * only for a non-BMP dash; so when {@code normalizeDashes} is enabled and the input contains a + * supplementary-plane dash, the returned spans are offsets into the normalized text rather than + * the original. For an exact original mapping in that case, use {@link #findInOriginal(String[])}.</p> * * @throws IllegalStateException Thrown if inference fails, if the model output shape is not * the expected {@code float[batch][token][label]} form, if the model output contains * no usable label score for a token, or if the model's predicted index for a token is not * present in the configured label map. - * @throws IllegalArgumentException Thrown if a token produced for the input is not present in - * the vocabulary, which indicates the vocabulary file does not match the model. + * @throws IllegalArgumentException Thrown if {@code input} is {@code null} or contains a + * {@code null} token, or if a token produced for the input is not present in the + * vocabulary, which indicates the vocabulary file does not match the model. */ @Override public Span[] find(String[] input) { + return locate(input).spans().toArray(new Span[0]); + } - final List<Span> spans = new ArrayList<>(); + /** + * Finds names and returns their {@link Span spans} in coordinates of the original joined input + * ({@code String.join(" ", input)}), regardless of any whitespace or dash normalization applied + * before inference. Spans are mapped back through the normalization {@link Alignment}, so a fold + * that changes the input length (a supplementary dash shrinking, or an expansion) does not shift + * the reported offsets. This implements {@link OffsetMappingNameFinder}, so an interface-typed + * caller can reach the offset-correct path with + * {@code finder instanceof OffsetMappingNameFinder}. + * + * @param input The tokens to search. + * @return The detected spans, in original-input character coordinates. + * @throws IllegalStateException Thrown under the same conditions as {@link #find(String[])}. + * @throws IllegalArgumentException Thrown under the same conditions as {@link #find(String[])}. + */ + @Override + public Span[] findInOriginal(String[] input) { + final DecodedSpans decoded = locate(input); + final Alignment alignment = decoded.aligned().alignment(); + final List<Span> mapped = new ArrayList<>(decoded.spans().size()); + for (final Span span : decoded.spans()) { + final Span original = alignment.toOriginalSpan(span.getStart(), span.getEnd()); + mapped.add(new Span(original.getStart(), original.getEnd(), span.getType(), span.getProb())); + } + return mapped.toArray(new Span[0]); + } + + // Shared core: normalize the joined input (capturing the alignment back to the original), then Review Comment: Converted the `//` block above `locate(...)` to a real javadoc comment with `@param`/`@return`/`@throws` ########## opennlp-core/opennlp-ml/opennlp-dl/src/main/java/opennlp/dl/namefinder/NameFinderDL.java: ########## @@ -153,69 +163,173 @@ public NameFinderDL(File model, File vocabulary, Map<Integer, String> ids2Labels this.includeTokenTypeIds = inferenceOptions.isIncludeTokenTypeIds(); this.documentSplitSize = inferenceOptions.getDocumentSplitSize(); this.splitOverlapSize = inferenceOptions.getSplitOverlapSize(); + this.normalizeWhitespace = inferenceOptions.isNormalizeWhitespace(); + this.normalizeDashes = inferenceOptions.isNormalizeDashes(); this.sentenceDetector = sentenceDetector; } private static InferenceOptions validateConstructorArguments( final InferenceOptions inferenceOptions, final Map<Integer, String> ids2Labels, final SentenceDetector sentenceDetector) { - Objects.requireNonNull(ids2Labels, "ids2Labels"); - Objects.requireNonNull(sentenceDetector, "sentenceDetector"); + requireNonNullArg(inferenceOptions, "inferenceOptions"); + requireNonNullArg(ids2Labels, "ids2Labels"); + requireNonNullArg(sentenceDetector, "sentenceDetector"); return inferenceOptions; } + /** * {@inheritDoc} * - * <p>This method joins the provided tokens with spaces, sentence-splits the joined text, - * runs each sentence through the ONNX token-classification model, decodes BIO labels into - * {@link Span spans}, and resolves those spans back to character offsets in the joined text.</p> + * <p>Joins the provided tokens with spaces, sentence-splits the joined text, runs each sentence + * through the ONNX token-classification model, decodes BIO labels into {@link Span spans}, and + * resolves those spans to character offsets in the joined text <em>after</em> any optional input + * normalization.</p> + * + * <p>Note: this returns correct original offsets in every case except one. Whitespace folding is + * length-preserving, so it never moves offsets. Only dash folding can change the input length, and + * only for a non-BMP dash; so when {@code normalizeDashes} is enabled and the input contains a + * supplementary-plane dash, the returned spans are offsets into the normalized text rather than + * the original. For an exact original mapping in that case, use {@link #findInOriginal(String[])}.</p> * * @throws IllegalStateException Thrown if inference fails, if the model output shape is not * the expected {@code float[batch][token][label]} form, if the model output contains * no usable label score for a token, or if the model's predicted index for a token is not * present in the configured label map. - * @throws IllegalArgumentException Thrown if a token produced for the input is not present in - * the vocabulary, which indicates the vocabulary file does not match the model. + * @throws IllegalArgumentException Thrown if {@code input} is {@code null} or contains a + * {@code null} token, or if a token produced for the input is not present in the + * vocabulary, which indicates the vocabulary file does not match the model. */ @Override public Span[] find(String[] input) { + return locate(input).spans().toArray(new Span[0]); + } - final List<Span> spans = new ArrayList<>(); + /** + * Finds names and returns their {@link Span spans} in coordinates of the original joined input + * ({@code String.join(" ", input)}), regardless of any whitespace or dash normalization applied + * before inference. Spans are mapped back through the normalization {@link Alignment}, so a fold + * that changes the input length (a supplementary dash shrinking, or an expansion) does not shift + * the reported offsets. This implements {@link OffsetMappingNameFinder}, so an interface-typed + * caller can reach the offset-correct path with + * {@code finder instanceof OffsetMappingNameFinder}. + * + * @param input The tokens to search. + * @return The detected spans, in original-input character coordinates. + * @throws IllegalStateException Thrown under the same conditions as {@link #find(String[])}. + * @throws IllegalArgumentException Thrown under the same conditions as {@link #find(String[])}. + */ + @Override + public Span[] findInOriginal(String[] input) { + final DecodedSpans decoded = locate(input); + final Alignment alignment = decoded.aligned().alignment(); + final List<Span> mapped = new ArrayList<>(decoded.spans().size()); + for (final Span span : decoded.spans()) { + final Span original = alignment.toOriginalSpan(span.getStart(), span.getEnd()); + mapped.add(new Span(original.getStart(), original.getEnd(), span.getType(), span.getProb())); + } + return mapped.toArray(new Span[0]); + } + + // Shared core: normalize the joined input (capturing the alignment back to the original), then + // decode each overlapping chunk bounded to its own character region and resolve overlaps. Bounding + // per chunk lets a boundary entity that two consecutive chunks both cover surface as overlapping + // candidates, which mergeOverlappingSpans collapses to the longer (more complete) span instead of + // silently keeping whichever a single forward cursor reached first. + private DecodedSpans locate(String[] input) { + + requireNonNullArg(input, "input"); + for (int i = 0; i < input.length; i++) { + if (input[i] == null) { + throw new IllegalArgumentException( + "The input must not contain null tokens; the token at index " + i + " was null."); + } + } // Join the tokens here because they will be tokenized using Wordpiece during inference. - final String text = String.join(" ", input); + final AlignedText normalized = + normalizeInputAligned(String.join(" ", input), normalizeWhitespace, normalizeDashes); + final String text = normalized.normalizedString(); // sentPosDetect (not sentDetect) so each sentence's offset in the full text is known. final Span[] sentenceSpans = sentenceDetector.sentPosDetect(text); + final List<Span> candidates = new ArrayList<>(); for (final Span sentenceSpan : sentenceSpans) { - // Floor the character cursor at this sentence's start, then thread it forward across the - // sentence's chunks so a repeated surface form is located at its next occurrence. Flooring - // per sentence keeps an entity from being matched against an identical surface form in an - // earlier sentence -- even one that produced no spans, which would otherwise leave the - // cursor behind and mis-locate the match. - int searchStart = sentenceSpan.getStart(); - - // The WordPiece tokenized text. This changes the spacing in the text. - final List<Tokens> wordpieceTokens = tokenize(sentenceSpan.getCoveredText(text).toString()); - - for (final Tokens tokens : wordpieceTokens) { - final List<Span> decoded = - decodeSpans(text, tokens.tokens(), infer(tokens), ids2Labels, searchStart, - sentenceSpan.getEnd()); - spans.addAll(decoded); - if (!decoded.isEmpty()) { - searchStart = decoded.get(decoded.size() - 1).getEnd(); - } + final int sentenceStart = sentenceSpan.getStart(); + final String sentence = sentenceSpan.getCoveredText(text).toString(); + + // The WordPiece tokenized text, in overlapping chunks. This changes the spacing in the text. + for (final ChunkTokens chunk : tokenize(sentence)) { + // Decode within the chunk's own character region in the full text. Keeping each chunk's + // entities inside the region it was built from locates a repeated surface form in the right + // chunk rather than mis-matching it to an earlier occurrence, while still letting two + // overlapping chunks both emit a boundary entity for mergeOverlappingSpans to reconcile. + final int regionStart = sentenceStart + chunk.start(); + final int regionEnd = sentenceStart + chunk.end(); + candidates.addAll(decodeSpans(text, chunk.tokens().tokens(), infer(chunk.tokens()), + ids2Labels, regionStart, regionEnd)); } } - return spans.toArray(new Span[0]); + return new DecodedSpans(mergeOverlappingSpans(candidates), normalized); + } + + private record DecodedSpans(List<Span> spans, AlignedText aligned) { + } + // A chunk's WordPiece tokens paired with the chunk's half-open character span in the full text. + private record ChunkTokens(Tokens tokens, int start, int end) { + } + + // Ordering for overlap resolution: longest span first, then higher probability. The dominant + // detection is kept and any later span overlapping it is dropped. + private static final Comparator<Span> BY_LENGTH_THEN_PROBABILITY = Review Comment: Moved `BY_LENGTH_THEN_PROBABILITY` up with the other `static final` constants -- 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]
