Skip to content

fix: JaroWinklerDistance of single character strings that differ in case - #786

Open
OsamaAnsar wants to merge 1 commit into
NaturalNode:masterfrom
OsamaAnsar:fix/jaro-winkler-short-strings
Open

OsamaAnsar wants to merge 1 commit into
NaturalNode:masterfrom
OsamaAnsar:fix/jaro-winkler-short-strings

Conversation

@OsamaAnsar

Copy link
Copy Markdown

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

What was wrong

JaroWinklerDistance returns a wrong value for strings of length 1 that are equal after lowercasing:

  • In distance() the match window is floor(max(len1, len2) / 2) - 1, which is -1 when both strings have length 1. No character can match, so the Jaro distance is 0.
  • In JaroWinklerDistance the prefix loop while (s1[l] === s2[l] && l < 4) compares undefined === undefined past the end of the strings, so it counts a prefix of 4 characters for strings that are shorter. With Jaro distance 0 this gives 0 + 4 * 0.1 * (1 - 0).

Real output on current master:

call before after
JaroWinklerDistance('A', 'a', { ignoreCase: true }) 0.4 1
JaroWinklerDistance('a', 'A', { ignoreCase: true }) 0.4 1
JaroWinklerDistance('I', 'i', { ignoreCase: true }) 0.4 1

Unchanged before and after: ('A', 'A') is 1, ('a', 'b') is 0, ('A', 'b', { ignoreCase: true }) is 0, ('a', 'ab') is 0.8500000000000001, ('ab', 'AB', { ignoreCase: true }) is 1, ('MARTHA', 'MARHTA') is 0.9611111111111111, ('DIXON', 'DICKSONX') is 0.8133333333333332, ('DWAYNE', 'DUANE') is 0.8400000000000001.

Proposed Changes

  • Clamp the match window with Math.max(0, ...). This is the change that fixes the example above (the window only goes negative when both strings have length 1).
  • Bound the prefix loop by the lengths of both strings. On its own this does not fix the example above (it would give 0.1); it only matters when a precomputed dj option is passed for strings shorter than 4 characters.
  • Added Jasmine tests in spec/jaro-winkler_spec.ts for strings of length 1 (with and without ignoreCase) and for the prefix bound with the dj option.

Verification

  • With only the change in lib/natural/distance/jaro-winkler_distance.js reverted (confirmed with git diff --stat and grep), the 2 new specs fail; with the change restored all 10 specs in jaro-winkler_spec pass.
  • npm test: 860 specs, 0 failures.
  • eslint . --ext .ts (the lint script): no errors. npx standard lib/natural/distance/jaro-winkler_distance.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)

For strings of length 1 the match window floor(1 / 2) - 1 is negative, so
no character matched and the Jaro distance was 0. The prefix loop also ran
past the end of the strings, so JaroWinklerDistance('A', 'a', { ignoreCase:
true }) returned 0.4 instead of 1. Clamp the match window to 0 and bound
the prefix loop by the lengths of the strings.

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