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

Fix nans in gfa #421

Merged
merged 3 commits into from Sep 24, 2014

Conversation

Projects
None yet
2 participants
@MrBago
Contributor

MrBago commented Sep 24, 2014

I've been wanting to fix this for months, I just never got to it. fit.gfa not returns 0 instead of nan when all the SH coefficients are 0 (almost exclusively in masked voxels). This should also get rid of one of the annoying warnings during the tests :).

return np.sqrt(1. - (coef_sq[..., sh0_index] / (coef_sq).sum(-1)))
numer = coef_sq[..., sh0_index]
denom = (coef_sq).sum(-1)
# Where all coefficients are zero

This comment has been minimized.

@stefanv

stefanv Sep 24, 2014

Contributor

Do you want to check explicitly where all coefficients are zero? If so, you should use np.all(coef_sq == 0). What you do here looks right to me, though, even though it doesn't match the comment.

This comment has been minimized.

@MrBago

MrBago Sep 24, 2014

Contributor

I believe np.sum(coef**2, axis=-1) == 0 is equivalent to np.all(coeff == 0, axis=-1). I put he comment here to so that anyone reading the code would notice this equivalence. How can a reword the comment to make that more clear?

This comment has been minimized.

@stefanv

stefanv Sep 24, 2014

Contributor

The sun being equal to zero is not the same as all the coefficients being
equal to zero. That is the part that confused me.

This comment has been minimized.

@MrBago

MrBago Sep 24, 2014

Contributor

Is the new comment better?

@MrBago MrBago force-pushed the MrBago:fix_nans_in_gfa branch from 51b062d to dd98bed Sep 24, 2014

@stefanv

This comment has been minimized.

Contributor

stefanv commented Sep 24, 2014

You mean, who is that idiot who didn't see the square :) Thanks, @MrBago

stefanv added a commit that referenced this pull request Sep 24, 2014

@stefanv stefanv merged commit a80acba into nipy:master Sep 24, 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