Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Serious bug in code word2vec. #1

Open
andy-soft opened this issue Sep 24, 2017 · 1 comment
Open

Serious bug in code word2vec. #1

andy-soft opened this issue Sep 24, 2017 · 1 comment

Comments

@andy-soft
Copy link

When you calculate the _expTable, in file Word2Vec.cs line 88 you use a bad issued integer division, and this yields always 0 if the numerator is less than numerator, which is always true, revise the calculated Exptable, all elements are the same. The solution is simple and is casting the ExpTableSize to double:
(other more elegant is to precompute is and define as a double constant, but this is used only on initialization) Also I would have done this as a static table, so if you use the class in a library more than once, you don't need to recalc the exptable every time (provided sizes are the same)
code from line 86 (aprox.)

for (var i = 0; i < ExpTableSize; i++)
            { 
                _expTable[i] = (float)Math.Exp((i / (double)ExpTableSize * 2 - 1) * MaxExp); // Precompute the exp() table
                _expTable[i] = _expTable[i] / (_expTable[i] + 1); // Precompute f(x) = x / (x + 1)
            }

NOTE: this kind of errors (integer divisions rendered as integer, and thought as floating point) are rather common on porting from C++, be careful, I am still searching the code for them!

GuntaButya added a commit that referenced this issue Feb 10, 2019
Bug Fix see: #1
@GuntaButya
Copy link
Owner

I have fixed and committed a new Push - Thank You for pointing out this error.

Gunt

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

No branches or pull requests

2 participants