fix(lyra): prevent NaN values in TF-IDF metric. Close #192 #193
Conversation
| // increase a token counter that may not yet exist | ||
| tokenOccurrencies[propName][token] = (tokenOccurrencies[propName][token] ?? 0) + 1; |
There was a problem hiding this comment.
Consider the following, taken from lyra.ts:
const propName = 'prop';
const token = 'doc-id';
const tokenOccurrencies = { [propName]: { } };What happens if we increase a non-existing counter like follows?
tokenOccurrencies[propName][token]++;Two things happen:
tokenOccurrencies[propName]becomes{ 'doc-id': NaN }NaNis returned from the postfix increment expression
If we wanted to increment a counter that may not yet exist, we should instead run something like:
tokenOccurrencies[propName][token] = (tokenOccurrencies[propName][token] ?? 0) + 1;which:
- sets the counter to 1 if it didn't exist
- increment the counter by 1 otherwise
There was a problem hiding this comment.
Damn you're right. Thank you so much!
| const termOccurrencies = typeof lyraOccurrencies[term] === "number" | ||
| ? (lyraOccurrencies[term] ?? 0) | ||
| : 0; |
There was a problem hiding this comment.
- anomalies in
termOccurrencies' value makeidfbecomeNaNorInfinity, withtfIdfresulting in aNaNvalue in turn - when
termOccurrenciesis a string, object, or undefined ->idfbecomesNaN->tfIdfbecomesNaN - when
termOccurrenciesis 0 ->idfbecomesInfinity->tfIdfbecomesNaN
| @@ -648,15 +649,22 @@ export function search<S extends PropertiesSchema>( | |||
| for (let j = 0; j < tokensLength; j++) { | |||
| const term = tokens[j]; | |||
| const documentIDs = getDocumentIDsFromSearch(lyra, { ...params, index, term, exact }); | |||
| const termOccurrencies = lyraOccurrencies[term]; | |||
|
|
|||
| // lyraOccurrencies[term] can be undefined, 0, string, or { [k: string]: number } | |||
There was a problem hiding this comment.
termOccurrencies's type is inferred asnumber, but it's actuallynumber | string | { [k: string]: number } | undefined
I recommend the maintainers to verify why this inferred type is misleading after this PR gets merged.
| // idf's denominator is shifted by 1 to avoid division by zero | ||
| const idf = Math.log10(N / (1 + termOccurrencies)); |
There was a problem hiding this comment.
This is such a common scenario that even wikipedia mentions this "unit shift" trick (link):
If the term is not in the corpus, this will lead to a division-by-zero. It is therefore common to adjust the denominator.
There was a problem hiding this comment.
This implies that the TF-IDF scores are now slightly smaller in the snapshot tests.
There was a problem hiding this comment.
That's right. Thanks a million for that, we used to add 1 and eventually removed it for testing purposes, but never re-introduced it again
There was a problem hiding this comment.
Looks good to me, thank you so much for these fixes @jkomyno!
| // increase a token counter that may not yet exist | ||
| tokenOccurrencies[propName][token] = (tokenOccurrencies[propName][token] ?? 0) + 1; |
There was a problem hiding this comment.
Damn you're right. Thank you so much!
| // idf's denominator is shifted by 1 to avoid division by zero | ||
| const idf = Math.log10(N / (1 + termOccurrencies)); |
There was a problem hiding this comment.
That's right. Thanks a million for that, we used to add 1 and eventually removed it for testing purposes, but never re-introduced it again
No description provided.