Repository navigation
Conversation
74ad23d to
9a87410
Compare
9a87410 to
1fc8ebe
Compare
rzo1
left a comment
rzo1
left a comment
There was a problem hiding this comment.
Little time, so here is a GPT 5.6-sol review instead for now
Request changes. The character-set optimization and null-pattern handling introduce regressions. The model manifest format remains unchanged.
Validation across the combined stack: 1,856 targeted tests, zero failures, one skipped.
| if (!isLiteral(to) || to < c) { | ||
| return null; | ||
| } | ||
| characters.set(c, to + 1); |
There was a problem hiding this comment.
A literal range spanning D800–DFFF includes surrogate code units in the lookup. It consequently accepts emoji that the original regex rejects. Please fall back to regex when a range intersects the surrogate block.
| Pattern alphaNumeric, TokenContextGenerator cg) { | ||
| super(tokenSamples); | ||
| this.alphaNumeric = alphaNumeric; | ||
| this.alphaNumeric = AlphaNumericCheck.of(alphaNumeric); |
There was a problem hiding this comment.
This now rejects a documented null pattern during construction, including when skipAlphaNumerics is false. That configuration previously worked. Please preserve it and add a regression test.
1fc8ebe to
586adc0
Compare
|
Here are some additional comments. The built-in patterns check out: all 9 give the same result as the regex on every BMP character, every supplementary code point, unpaired surrogates and 200k random tokens each, and tokenization of the released en/de 1.3.0 models is unchanged. Def. still needs the tokenizer eval build, since Blocking
Minor
Verified: thread safety is fine (final fields, BitSet read-only after construction), no public API or serialization change, and a model with no pattern still resolves to the language's built-in class, as before. |
|
Correction to my previous comment: the |
980deac to
202309b
Compare
krickert
changed the title
f84acf8 to
9c33819
Compare
rzo1
changed the title
rzo1
changed the title
|
Please rebase onto main. #1275 is closed and CompatibilityMode is in main via #1312; since b3c79c1 this PR no longer uses it, so only the OPENNLP-1934 commits are needed. Please also update the description ("Based on #1275" and the LEGACY surrogate note). |
b3c79c1 to
9ca624b
Compare
…ter set
The alphanumeric pattern stays a Pattern in TokenizerFactory and
lang.Factory: it is written to the model manifest as a regex string and
read back with Pattern.compile, so the type is part of the model contract.
A package-private AlphaNumericCheck reads a pattern of the form ^[...]+$
with a class of plain characters and simple ranges, the form of every
built-in language pattern, into a CodePointSet and tests tokens by lookup.
Any other pattern, and any pattern compiled with flags, is evaluated by the
regular expression engine. TokenizerME and TokSpanEventStream hold the
check instead of the Pattern and build it only when the optimization is on.
A token is alphanumeric when every character in it is in the class the
pattern defines. An unpaired surrogate is not a character, so a token that
is not well-formed UTF-16 is not alphanumeric under any pattern: the
check walks the token by code point before either evaluation, and such a
token goes to the model at inference and keeps its events in training. The
regular expression engine's own acceptance of a lone surrogate inside a
range is an artifact of how it walks code units and is not copied.
Behavior changes:
- TokenizerME with the optimization on and a factory that returns
null from getAlphaNumericPattern() threw NullPointerException at the
first token; it now uses Factory.DEFAULT_ALPHANUMERIC.
- TokSpanEventStream with a null pattern and skipAlphaNumerics=true threw
NullPointerException; null now means Factory.DEFAULT_ALPHANUMERIC, as
the constructor Javadoc stated.
- A custom pattern that accepts a lone surrogate code unit (a range over
the surrogate block, a negated class, \p{Cs}, or .) let a malformed
token skip the model and lose its training events. No built-in pattern
accepts one, so well-formed input is unchanged.
Tests use the regex as the oracle for well-formed characters: the set
lookup is compared with it on every code point from U+0000 to U+10FFFF
for each built-in language pattern and for 21 set-eligible custom
patterns. They pin 17 pattern shapes to the engine and cover whole-token,
flag, null, and malformed-token cases, events from TokSpanEventStream,
and context counts in TokenizerME. The manual adds
an "Alphanumeric optimization" section in the tokenizer chapter.
Red evidence, tests in place: with the null substitution removed,
TokSpanEventStreamTest.testNullPatternMeansAsciiDefault and
TokenizerMETest.testNullAlphaNumericPatternMeansAsciiDefault[optimization
on] error with "pattern must not be null"; with the surrogate check
removed, 13 of 131 targeted cases fail (10 in AlphaNumericCheckTest, 2 in
TokSpanEventStreamTest, 1 in TokenizerMETest). All pass with both.
Eval: ArvoresDeitadasEval tokenizer cross-validation (pt, optimization
on, five trainer variants) at the pinned F-measures, and
SourceForgeModelEval.evalTokenModel with the output hash unchanged.
9ca624b to
263999e
Compare
Prepare the alphanumeric checker regardless of the stored model setting so subclasses can enable or disable the shortcut safely. Reject null checker tokens with IllegalArgumentException and retain malformed Unicode handling. Four regressions failed before the fix. Full runtime verification, 156 affected tokenizer tests, Portuguese GIS cross-validation, the SourceForge tokenizer evaluation, Javadoc and the manual build pass.
|
Keep this in draft, it's incomplete |
|
This entire PR is cheating and it isn't performant. Closing to open additional PRs to fix it right. |
|
I'll reopen this when it's ready. This PR still had a large regex surface, but I'm planning out how to properly fix this by looking at the other algorithms out there and NLP packages that do it right. We'll take the best of all world.. It'll be about (SMALL!) 9 PRs total - promise to be human measurable. Functionality will remain intact with each one, I recommend we try but I don't wanna burn out the review cycle - so I will work hard to ensure they are small and quickly reviewable. Not my fault :) It's just the code surface and the profiler guiding me. This doesn't change the other PRs, it's just missed areas I didn't see before. |
krickert
changed the title
2 participants
Add an immutable
TokenizerCharacterPolicythat checks complete candidates against explicit letter, digit and mark sets using a code-point scan. Combining marks must follow letters, supplementary characters are supported, and malformed UTF-16 is rejected. The factory validates category sets and provides an ASCII preset. Tests and the manual cover the contract.This PR introduces the policy API only. Connecting it to tokenizer training, inference and model persistence, and adding Unicode presets, are separate follow-ups. It does not yet replace the existing tokenizer regex configuration.
Validation on main at
c36ff62f7:OPENNLP-1934