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

Fixed initial estimation in piesno #683

Merged
merged 3 commits into from Jul 27, 2015

Conversation

Projects
None yet
3 participants
@samuelstjean
Contributor

samuelstjean commented Jul 18, 2015

Turns out I misread something the first time, this fixes the initial estimation fo sigma for piesno. The current version uses the optimal quantile of the current slice, whereas the original paper suggest to use the full volume instead. This should hopefully fix this little discrepancy.

piesno_fix

@arokem

This comment has been minimized.

Member

arokem commented Jul 18, 2015

First pass impression: this requires some additional tests. In particular, a test that exercises the new m kwarg.

@arokem

This comment has been minimized.

Member

arokem commented Jul 18, 2015

Also maybe rename that kwarg to something more meaningful

@@ -13,6 +12,18 @@ def _inv_nchi_cdf(N, K, alpha):
return gammainccinv(N * K, 1 - alpha) / K
# List of optimal quantil for PIESNO.

This comment has been minimized.

@Garyfallidis

Garyfallidis Jul 18, 2015

Member

e missing

@arokem

This comment has been minimized.

Member

arokem commented Jul 20, 2015

Also, out of curiosity, a question: how much of a difference does this make? That is roughly how much do the estimates with this fix differ from estimates without the fix?

sigma, mask = piesno(rician_noise, N=1, alpha=0.01, l=1, eps=1e-10, return_mask=True)
# less than 3% of error?
assert_almost_equal(np.abs(sigma - 50) / sigma < 0.03, True)

This comment has been minimized.

@arokem

arokem Jul 20, 2015

Member

In particular, do these tests fail without the fix?

This comment has been minimized.

@samuelstjean

samuelstjean Jul 20, 2015

Contributor

they should still work, this test is for the 4D convenience function, the 3D function itself behaves in the same way.

@arokem

This comment has been minimized.

Member

arokem commented Jul 20, 2015

Not complaining, just curious. Other than that, this looks good to me.

@samuelstjean

This comment has been minimized.

Contributor

samuelstjean commented Jul 20, 2015

From some tests it is exactly the same, it's pretty robust to the initialisation. It should also fix the case where 0 was returned when a low number of noise voxels was found. Since this only provides a upper bound on the highest possible value, this only matters when this highest bound was too low for a single 3D slice.

@arokem

This comment has been minimized.

Member

arokem commented Jul 20, 2015

Good. I'll give others the opportunity to take a look, and merge it in a week or so, if no other comments arise.

arokem added a commit that referenced this pull request Jul 27, 2015

Merge pull request #683 from samuelstjean/piesno_median_fix
Fixed initial estimation in piesno

@arokem arokem merged commit 647ea47 into nipy:master Jul 27, 2015

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details

@samuelstjean samuelstjean deleted the samuelstjean:piesno_median_fix branch Jul 27, 2015

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment