Skip to content

fix: do not match abbreviations inside other words in SentenceTokenizer - #784

Open
OsamaAnsar wants to merge 1 commit into
NaturalNode:masterfrom
OsamaAnsar:fix/sentence-tokenizer-abbreviations
Open

OsamaAnsar wants to merge 1 commit into
NaturalNode:masterfrom
OsamaAnsar:fix/sentence-tokenizer-abbreviations

Conversation

@OsamaAnsar

Copy link
Copy Markdown

Fixes # (no existing issue; the problem is described below)

What was wrong

SentenceTokenizer.replaceAbbreviations builds its pattern as an alternation of the configured abbreviations with the gi flags 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. With new SentenceTokenizer(['Mr.', 'Dr.', 'St.', 'e.g.', 'Inc.']):

input before after
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

  • Prefix the abbreviation alternation with the lookbehind (?<![\p{L}\p{N}]) and add the u flag (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 by escapeRegExp, which stays valid under the u flag. Lookbehind is already used elsewhere in lib (e.g. porter_stemmer_uk.js, sentence_tokenizer_deprecated.js), and the CI matrix is Node 20, 22 and 24.
  • Add Jasmine tests in spec/sentence_tokenizer_spec.ts: the three cases above, a word ending in a non-ASCII letter before st. (Swedish "röst."), and checks that Mr., Dr., e.g. (also after a comma or an opening bracket), a standalone St. and upper-case MR. are still treated as abbreviations. Outputs for the "after" column were obtained by running the tokenizer on the changed code.
  • Behaviour that is unchanged: Mr. Smith went to Washington. He liked it. and We 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

  • With only the change in lib/natural/tokenizers/sentence_tokenizer.js reverted (confirmed with git diff --stat and 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 (the lint script): no errors. npx standard lib/natural/tokenizers/sentence_tokenizer.js: no errors.
  • npm run test_io was not run (it needs external services).

Checklist before merging

  • Added Jasmine tests
  • Updates TypeScipt index.d.ts files (not applicable: no API or type change, index.d.ts is untouched)

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

No deployments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant