Undo the hash2index optimization

[mpenkov]

This optimization reduced the number of ngram buckets to include only ngrams that we have seen during training.

This seemed like a good idea at the time, because it saved CPU cycles and RAM, but turned out to be a bad idea, because it introduced divergent behavior when compared to the reference implementation. For example:

We were unable to calculate vectors for terms that were completely out of the vocab (so the term and all its ngrams were unseen). This is bad because the original FB implementation always returns a vector. It may seem useless because it's initialized to a random vector, but that's not entirely true, because that vector is random at initialization time. When we're querying the ngram's vector, the vector is deterministic, so it is useful.

Another problem is that it complicated the implementation. We now needed an additional layer of indirection that mapped hashes to bucket indices. Without this optimization, this mapping is essentially the identifiy function: the hash N always maps to the Nth bucket.

This pull request removes the optimization, resolving the problems that it introduced.

Fixes #2329

[mpenkov]

@piskvorky When do you think would be the best time to release this?

We can sneak it into 3.7.2 (the next bugfix release) or wait until the next minor release (3.8.0). Technically, this isn't really a bugfix, it's a feature improvement (more precisely, the removal of a bad feature), so it can wait.

WDYT?

[piskvorky]

I'd consider it a bug fix, and release as soon as possible. But #2371 NMF still blocking.

[gojomo]

Correcting a deviation from the behavior of Facebook's reference FastText implementation definitely seems like a bug fix to me!

And it would be good to include a release-note advisory about the changed behavior, now that gensim's FastText (post-fix) will always return a vector for OOV words (and never the "all ngrams for word X absent from model" error it was throwing previously).

[mpenkov]

Already done in the changelog on develop :)

[gojomo]

@mpenkov I see a note about __contains__, but it's a little confusing. Isn't __contains__() and the Python built-in in operator supposed to give identical results? That's actually specified by the Python 3.7 documentation, but the current text of the changelog suggests gensim will do the opposite: always return True for __contains__(), but sometimes return False for in. I see now that the difference is checking the .wv.vocab property; that should be emphasized moreso than a __contains__ vs in approach.

I don't see a note that all [] lookups will now always return a vector, and never return the KeyError they sometimes did, for no available character n-grams, in gensim through 3.7.1. It would help to note this explicitly, and that this is being done to match the reference FastText behavior.

[mpenkov]

OK, please have a look at this commit and let me know if it is sufficient.

[gojomo]

Put an extensive note on the commit.