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 fix float overflow in estimate_sigma #504

Merged
merged 2 commits into from Dec 15, 2014

Conversation

Projects
None yet
3 participants
@Garyfallidis
Member

Garyfallidis commented Dec 14, 2014

Hi I noticed, that in HCP data the sigma was all zeros. This change fixes the problem. GGT!

@arokem

This comment has been minimized.

Member

arokem commented Dec 14, 2014

A bit more detail, please? What problem does this solve exactly?

In particular, there are a few other arrays that are being allocated further up in this same function. Do they also have to be allocated as float64?

@samuelstjean

This comment has been minimized.

Contributor

samuelstjean commented Dec 14, 2014

The problem was that if you are really unlucky, since scipy stores the intermediate computation as the same datatype as the output array, elef managed to overflow 10**38. It's the same bug that you had with the stanford T1 overflowing int16.

Since the convolution is the sum of a 3x3x3 kernel region, it was sometime overflowing and giving back negatives values, hence the problem. Now it's up at 10**308, so I would be surprised it breaks this time.

The other arrays are fine, the problem is the way scipy stores the intermediate array when you specify one. Unless your dataset uses the full dynamic range of float32 (which is 10**38), you shouldn't have any problem.

@Garyfallidis

This comment has been minimized.

Member

Garyfallidis commented Dec 14, 2014

@arokem are you happy with the clarification? The quick answer here is that "No the other arrays do not need to be float64".

@arokem

This comment has been minimized.

Member

arokem commented Dec 15, 2014

Good - just wanted to make sure that we don't have to go looking for this at some point in the future.

arokem added a commit that referenced this pull request Dec 15, 2014

Merge pull request #504 from Garyfallidis/bf_float_overflow
Bug fix float overflow in estimate_sigma

@arokem arokem merged commit 5bea5e7 into nipy:master Dec 15, 2014

1 check passed

continuous-integration/travis-ci The Travis CI build passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment