Repository navigation
fix: do not match abbreviations inside other words in SentenceTokenizer - #784
Open
OsamaAnsar wants to merge 1 commit into
Open
OsamaAnsar wants to merge 1 commit into
OsamaAnsar wants to merge 1 commit into
Conversation
The abbreviation pattern had no word boundary, so with abbreviations such as "St." and "Inc." configured, the end of "best.", "list." or "zinc." was taken for an abbreviation and the sentence was not split. Only match an abbreviation when it does not directly follow a letter or a digit.
This branch has not been deployed
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixes # (no existing issue; the problem is described below)
What was wrong
SentenceTokenizer.replaceAbbreviationsbuilds its pattern as an alternation of the configured abbreviations with thegiflags and no word boundary. An abbreviation therefore also matches at the end of an ordinary word, is swallowed as an abbreviation, and the sentence is not split there. Withnew SentenceTokenizer(['Mr.', 'Dr.', 'St.', 'e.g.', 'Inc.']):This is the best. Next one starts here.["This is the best. Next one starts here."]["This is the best.","Next one starts here."]We love zinc. It is a metal.["We love zinc. It is a metal."]["We love zinc.","It is a metal."]See the list. Then go.["See the list. Then go."]["See the list.","Then go."]Best. Next.["Best. Next."]["Best.","Next."]("best." contains "st.", "zinc." contains "Inc." and "list." contains "St.", matched case-insensitively.)
Proposed Changes
(?<![\p{L}\p{N}])and add theuflag (needed for\p{...}), so an abbreviation only matches when it does not directly follow a letter or a digit. The abbreviations were already regex-escaped byescapeRegExp, which stays valid under theuflag. Lookbehind is already used elsewhere inlib(e.g.porter_stemmer_uk.js,sentence_tokenizer_deprecated.js), and the CI matrix is Node 20, 22 and 24.spec/sentence_tokenizer_spec.ts: the three cases above, a word ending in a non-ASCII letter beforest.(Swedish "röst."), and checks thatMr.,Dr.,e.g.(also after a comma or an opening bracket), a standaloneSt.and upper-caseMR.are still treated as abbreviations. Outputs for the "after" column were obtained by running the tokenizer on the changed code.Mr. Smith went to Washington. He liked it.andWe met Dr. Who. He was nice.still split as before; I compared the tokenizer output on 15 sample sentences before and after the change and only the four rows above differ.Verification
lib/natural/tokenizers/sentence_tokenizer.jsreverted (confirmed withgit diff --statand grep), 4 of the new specs fail (best, zinc, list, non-ASCII); with the change restored the sentence tokenizer spec file passes (35 specs, 0 failures).npm test: 867 specs, 0 failures.eslint . --ext .ts(thelintscript): no errors.npx standard lib/natural/tokenizers/sentence_tokenizer.js: no errors.npm run test_iowas not run (it needs external services).Checklist before merging
index.d.tsfiles (not applicable: no API or type change,index.d.tsis untouched)