Skip to content

fix: do not look up TfIdf terms on the prototype of documents and cache - #785

Open
OsamaAnsar wants to merge 1 commit into
NaturalNode:masterfrom
OsamaAnsar:fix/tfidf-prototype-keys
Open

OsamaAnsar wants to merge 1 commit into
NaturalNode:masterfrom
OsamaAnsar:fix/tfidf-prototype-keys

Conversation

@OsamaAnsar

Copy link
Copy Markdown

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

What was wrong

Terms that are also property names of Object, such as constructor, were looked up on the prototype chain in lib/natural/tfidf/tfidf.js:

  • TfIdf.tf is document[term] ? document[term] : 0. On a plain-object document that lacks the term, document['constructor'] is the Object function, so tf returns a function and tfidf() returns NaN.
  • The constructor and addFileSync set this._idfCache = {}. With a plain-object cache, idf('constructor') finds Object in the cache and returns the function instead of a number. This hits instances created from deserialized data (new TfIdf(json)) and instances after addFileSync() (addDocument and removeDocument already use Object.create(null); issue Tfidf scoring doesnt work for the word 'constructor' #119 only patched the document-building side).

Real output on current master (3 documents: this document is about node and javascript, this document is about ruby and rails, the constructor is a special method):

call before after
tfidf('constructor', 0) (term absent) NaN 0
tfidfs('constructor') [NaN, NaN, 1.4054651081081644] [0, 0, 1.4054651081081644]
tfidf('__proto__', 0) NaN 0
deserialized: typeof idf('constructor') function number
deserialized: tfidf('constructor', 0) (absent) NaN 0
deserialized: tfidf('constructor', 2) (present) NaN 1.4054651081081644
after addFileSync: typeof idf('constructor') function number
after addFileSync: tfidf('constructor', 1) (present) NaN 1

Ordinary terms (e.g. node) give identical results before and after. (tfidfs returns NaN; JSON.stringify of it prints null.)

Proposed Changes

  • TfIdf.tf and documentHasTerm only consider own properties of the document (Object.prototype.hasOwnProperty.call).
  • The constructor and addFileSync create the IDF cache with Object.create(null), as addDocument and removeDocument already do.
  • Added Jasmine tests in spec/tfidf_spec.ts for: tf of an absent reserved name, tfidf of 0 (not NaN) for absent reserved names (toString, valueOf, hasOwnProperty, constructor, __proto__, ...), a present constructor term, listTerms with no NaN, a deserialized instance and the cache after addFileSync().

Verification

  • With only the change in lib/natural/tfidf/tfidf.js reverted (confirmed with git diff --stat and grep), 4 of the new specs fail (the tf, tfidf of absent terms, deserialized and addFileSync ones); the other new specs are regression guards that pass either way. With the change restored, all 27 specs in tfidf_spec pass.
  • npm test: 864 specs, 0 failures.
  • eslint . --ext .ts (the lint script): no errors. npx standard lib/natural/tfidf/tfidf.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)

Terms that match a property of Object, such as "constructor", were looked
up on the prototype chain. TfIdf.tf returned the Object function for a
document that lacks the term, so tfidf() returned NaN, and the IDF cache
created by the constructor and addFileSync was a plain object, so idf()
returned the Object function for instances created from deserialized data
or filled with addFileSync.

Use own property lookups for the term frequency and use a cache without a
prototype in the constructor and in addFileSync, like addDocument does.

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