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

BUG: stable evaluation of expit #3386

Merged
merged 5 commits into from
Feb 25, 2014
Merged

Conversation

argriffing
Copy link
Contributor

closes #3385

@@ -73,3 +73,8 @@ def test_float64(self):
0.79139147, 0.9022274,
0.95734875, 0.98201379])
self.check_expit_out('f8', expected)

def test_float64_large(self):
yield assert_allclose, 1.0, expit(710)
Copy link
Member

Choose a reason for hiding this comment

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

yield tests don't work in TestCase subclasses (nose weirdness...).

@argriffing
Copy link
Contributor Author

Fixed the yield assertions. I didn't know they weren't allowed in TestCase, and it's disconcerting to me that they were just silent. I've probably committed some code in scipy or elsewhere with tests that are not doing anything...

@argriffing
Copy link
Contributor Author

Is this the kind of bug that could be backported?

@pv pv added this to the 0.14.0 milestone Feb 25, 2014
@pv
Copy link
Member

pv commented Feb 25, 2014

Yes. However, @rgommers didn't branch 0.14.0 yet.

@coveralls
Copy link

Coverage Status

Coverage remained the same when pulling 5a4fd30 on argriffing:expit-evaluation into 48b9d09 on scipy:master.

@coveralls
Copy link

Coverage Status

Coverage remained the same when pulling 5a4fd30 on argriffing:expit-evaluation into 48b9d09 on scipy:master.

@argriffing
Copy link
Contributor Author

I hope this fix is easy to review. Both conditional branches are mathematically the same, but they are numerically stable in different regimes (0<x vs x<0). The expit function is also called the logistic function (which is different from logit). See this blog post explaining how to evaluate it in a numerically stable way, branching according to the sign of x.

@WarrenWeckesser
Copy link
Member

Looks good to me. There should also be tests for other data types, since the same problem occurs (at different thresholds) for float32 and float128:

In [50]: expit(np.array([88, 89], dtype=np.float32))
Out[50]: array([  1.,  nan], dtype=float32)

In [51]: expit(np.array([11356, 11357], dtype=np.float128))
Out[51]: array([ 1.0,  nan], dtype=float128)

@argriffing
Copy link
Contributor Author

btw numpy and scipy are littered with bugs for float > 64 right? I never use these precisions, and the only time I see them is for reported bugs, so I assume that they are not truly supported beyond numpy.ndarray being able to physically store their representations.

@argriffing
Copy link
Contributor Author

also my platform doesn't even have np.float96. Are all platforms guaranteed to have np.float128?

@argriffing
Copy link
Contributor Author

@WarrenWeckesser added

@rgommers
Copy link
Member

@pv correct. I planned to last night but there were too many PRs to merge the past two days. And at least I'd like to fix part of gh-3330; it makes little sense to produce a beta if I get a couple of hundred test errors myself.

pv added a commit that referenced this pull request Feb 25, 2014
BUG: special: fix stable evaluation of expit
@pv pv merged commit 77a7e48 into scipy:master Feb 25, 2014
@pv
Copy link
Member

pv commented Feb 25, 2014

Thanks, LGTM

@larsmans
Copy link
Contributor

larsmans commented Mar 3, 2014

I'm still surprised that it's evaluated this way. In scikit-learn, we had this implementation (in Cython), until I figured out that reducing expit to tanh was faster and stable enough (up to x=1000, at least):

def expit(x):
    z = .5 * x
    np.tanh(z, z)
    z += 1
    z *= .5
    return z

Has this ever been considered for the scipy.special implementation?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
defect A clear bug or issue that prevents SciPy from being installed or used as expected scipy.special
Projects
None yet
Development

Successfully merging this pull request may close these issues.

expit does not handle large arguments well
7 participants