Check if margin > 0, not if prob > 0.5

[naftaliharris]

This avoids an unnecessary computation and also possible math overflow errors.

[AmplabJenkins]

Can one of the admins verify this patch?

[mateiz]

@mengxr what do you think about this?

[mateiz]

Actually the Python version of this code seems like it needs to be brought in line with the threshold API added to Java / Scala. After that, the change won't work any more, though it is correct for threshold = 0.5.

[mengxr]

I think we should keep it as it is now and add support for setting thresholds.

[naftaliharris]

My point actually is that you get OverflowError's if margin is too large, for example:

>>> from math import exp
>>> margin = -1000
>>> prob = 1/(1 + exp(-margin))
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
OverflowError: math range error
>>>

This actually happened to me, which is how I saw this issue.

There are (at least) two ways to solve this: One is to check margin instead of prob; this has the advantage of also saving a computation. Even if you allow the user to specify the probability threshold p at some later date, you can just change the margin cutoff from 0 to log(p/(1-p)).

The second solution is to replace math.exp with np.exp, which gives a warning rather than an exception:

>>> import numpy as np
>>> margin = -1000
>>> prob = 1/(1 + np.exp(-margin))
__main__:1: RuntimeWarning: overflow encountered in exp
>>> prob
0.0
>>>

For robustness of the code, perhaps you should at least do that.

Thanks!

[mateiz]

That makes sense. I believe this was fixed just recently in here though: #1493. Does the fix look okay to you?

[naftaliharris]

Yeah, that fixes the overflow errors! The second probability calculation has some slight numerical issues, (see in-line comment), but not a huge deal.

[mateiz]

Hey @naftaliharris, might closing this pull request now that this has been fixed in other PRs?

[naftaliharris]

@mateiz oh yeah, no problem. Thanks again for the fixes!