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

Optimising sigmoid function everywhere #990

Merged
merged 5 commits into from
Nov 8, 2016
Merged

Conversation

markroxor
Copy link
Contributor

No description provided.

return sum(lprob)


def score_cbow_pair(model, word, word2_indices, l1):
l2a = model.syn1[word.point] # 2d matrix, codelen x layer1_size
sgn = (-1.0)**word.code # ch function, 0-> 1, 1 -> -1
lprob = -log(1.0 + exp(-sgn*dot(l1, l2a.T)))
lprob = log(expit(sgn * dot(l1, l2a.T)))
Copy link
Owner

@piskvorky piskvorky Nov 1, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not directly related to this PR, but since we're on the topic, there's a function in numpy for safely adding up logs of exps: numpy.logaddexp(0, x).

@piskvorky
Copy link
Owner

piskvorky commented Nov 1, 2016

These are all welcome @markroxor , but I wonder why such overflows/underflows happen in the first place.

Making exp(1000) or exp(-1000) "work" is probably just masking the real underlying problem, the root cause. Why should the exponents in word2vec be so huge?

@markroxor
Copy link
Contributor Author

I've never encountered a case when such a warning might occur but it was reported in issue #838 (it depends on the corpus I suppose). AFAIK we are just handling the huge exponents in a better way so it should not cause any problem.

@gojomo
Copy link
Collaborator

gojomo commented Nov 1, 2016

Using a scipy function seems like a good idea. However, note these pure-Python paths are not tested by our automated testing (because the Cythonized routines are available at that point), so such changes need careful testing by other more manual-means. @markroxor, have you run the Word2Vec/Doc2Vec test suites locally with the optimized-code disabled?

@piskvorky
Copy link
Owner

piskvorky commented Nov 2, 2016

AFAIK we are just handling the huge exponents in a better way so it should not cause any problem.

Well, that's the part I doubt. The fact that we now return 0.0 or inf or -inf correctly, without an overflow/underflow warning, doesn't really qualify as "no problem" IMO. It's possible it just pushes the problem further upstream.

Your changes are nice and welcome @markroxor (and LGTM now); this is a more a note to self and @tmylk , to investigate and fix this more thoroughly.

@markroxor
Copy link
Contributor Author

@gojomo "with the optimized-code disabled". Do you mean that I should test word2vec/doc2vec without incorporating these changes?

@gojomo
Copy link
Collaborator

gojomo commented Nov 2, 2016

@markroxor - the change you've made is in code that's only run if the Cython-optimized versions (word2vec_inner.pyx, doc2vec_inner.pyx) are not available. In a normal installation, or in the Travis-CI tests, those versions are available. So the changed code won't be run unless you do something to disable the optimized code - like changing the load-attempt at the top of doc2vec.py or word2vec.py.

At times in the past to test the pure-Python paths, I've commented-out that load, then run a full test suite locally. It might be nice if the classes allowed a user-switch without editing code. But, without the optimization tests that normally take seconds take many minutes, so as a practical matter the pure-Python paths may never be part of the automated test suite.

@markroxor
Copy link
Contributor Author

@gojomo I executed the tests locally and they passed perfectly. Indeed the execution was very slow.

@tmylk tmylk merged commit dae481e into piskvorky:develop Nov 8, 2016
@markroxor markroxor deleted the 895 branch December 22, 2017 05:35
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.

4 participants