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

RF: Use the average sigma in the mask. #1166

Merged
merged 7 commits into from Apr 9, 2017

Conversation

Projects
None yet
5 participants
@arokem
Member

arokem commented Dec 17, 2016

Instead of the value of sigma in the corner of the image.

Addresses #1131

@arokem

This comment has been minimized.

Member

arokem commented Dec 17, 2016

Urgh. What's the right way to handle nanmean? It's not in scipy.stats in newer versions of scipy, and it's not in numpy in older versions of numpy. Do I really have to check the version of both numpy and scipy to be sure that I can compute a nanmean?

@MarcCote

This comment has been minimized.

Contributor

MarcCote commented Jan 7, 2017

Maybe compute it explicitly instead of relying on nanmean? See http://stackoverflow.com/questions/5480694/numpy-calculate-averages-with-nans-removed for some variants.

NB: Maybe a bit slower compared to np.nanmean, though.

@codecov-io

This comment has been minimized.

codecov-io commented Jan 7, 2017

Codecov Report

Merging #1166 into master will increase coverage by <.01%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #1166      +/-   ##
==========================================
+ Coverage   85.87%   85.87%   +<.01%     
==========================================
  Files         221      221              
  Lines       27121    27128       +7     
  Branches     2776     2777       +1     
==========================================
+ Hits        23290    23297       +7     
  Misses       3148     3148              
  Partials      683      683
Impacted Files Coverage Δ
dipy/denoise/non_local_means.py 100% <100%> (ø) ⬆️
dipy/denoise/tests/test_ascm.py 97.01% <100%> (ø) ⬆️
dipy/denoise/tests/test_non_local_means.py 96.72% <100%> (+0.56%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 520b724...d97a429. Read the comment docs.

@arokem arokem force-pushed the arokem:non_local_means_sigma branch from bb0861a to 48ab1c4 Jan 7, 2017

@arokem

This comment has been minimized.

Member

arokem commented Jan 7, 2017

Thanks! That looks like a good solution to me. We can check whether numpy has a nanmean and use that one if possible, and otherwise use our own. What do you think?

@MarcCote

This comment has been minimized.

Contributor

MarcCote commented Jan 7, 2017

I liked it. 👍

@arokem

This comment has been minimized.

Member

arokem commented Jan 7, 2017

What do you think about all the rest of this? Does it properly address #1131 in your opinion?

@arokem arokem force-pushed the arokem:non_local_means_sigma branch 3 times, most recently from 3424c19 to 40149a5 Jan 8, 2017

@coveralls

This comment has been minimized.

coveralls commented Jan 8, 2017

Coverage Status

Coverage decreased (-0.01%) to 88.378% when pulling 40149a5 on arokem:non_local_means_sigma into d489c86 on nipy:master.

@arokem

This comment has been minimized.

Member

arokem commented Jan 24, 2017

Any more thoughts here? Seems like a bug we need to address before we can make a release.

@arokem

This comment has been minimized.

Member

arokem commented Feb 3, 2017

Anyone want to take a look at this?

@MarcCote

This comment has been minimized.

Contributor

MarcCote commented Feb 9, 2017

This looks good to me. @samuelstjean last chance to have a look at this PR. Does it address your issue #1131 correctly?

@samuelstjean

This comment has been minimized.

Contributor

samuelstjean commented Feb 9, 2017

It kinds of circumvent the problem. It is a less brute approach than picking the top left corner voxel, but it does cause problem if you have a large variation of the noise profile in your image. In that case, I would prefer if it just threw me an error that 3D arrays are not supported, and let me choose how to deal with it.

For example, say I collected a noise measurement maps, extract local standard deviation from that, and feed it to the function. It will pick out the mean, and if the map is first masked (say by the scanner), then the median would probably be more appropriate to deal with those 3/4 of zeros values. As of now, I'll probably get a useless value, where the previous nlmeans version would happily work with my 3D map at each spatial location. Since that is now the case anymore, this would all happen silently and give me a less optimal result than I expected (true story, ask JC).

@arokem

This comment has been minimized.

Member

arokem commented Mar 14, 2017

I am back to this. I agree with @samuelstjean. It doesn't make sense to accept 3D sigma inputs to this function, because it will create a scalar under the hood using less than obvious rules. If you want to extract your sigma from a 3D volume, please do so before you feed it to this function. Simplified accordingly.

@coveralls

This comment has been minimized.

coveralls commented Mar 15, 2017

Coverage Status

Coverage increased (+0.03%) to 88.418% when pulling 204f94f on arokem:non_local_means_sigma into d489c86 on nipy:master.

@arokem arokem force-pushed the arokem:non_local_means_sigma branch from 204f94f to d321414 Mar 23, 2017

@arokem

This comment has been minimized.

Member

arokem commented Mar 23, 2017

Now rebased on master

@coveralls

This comment has been minimized.

coveralls commented Mar 23, 2017

Coverage Status

Coverage increased (+0.08%) to 88.509% when pulling d321414 on arokem:non_local_means_sigma into dd4a17b on nipy:master.

@MarcCote

This comment has been minimized.

Contributor

MarcCote commented Apr 3, 2017

@arokem @samuelstjean I'll be merging this PR on Monday.

@samuelstjean

This comment has been minimized.

Contributor

samuelstjean commented Apr 3, 2017

  • the nanmean function serves no purpose
  • not sure what error message it will throw you if you pass a 3D array, and it might be hard to decipher

not sure it is fully finished in this current form, but the logic seems there

@arokem

This comment has been minimized.

Member

arokem commented Apr 3, 2017

@arokem arokem force-pushed the arokem:non_local_means_sigma branch from f5ea84d to 3d0ddfd Apr 4, 2017

@arokem

This comment has been minimized.

Member

arokem commented Apr 4, 2017

I added the option of passing in an array, if that array is a singleton. This fixes the test failures from yesterday (at least on my machine), and actually seems reasonable to me. I don't think that we need to adjust the documentation. The canonical input is still a single float, it's just that if you happen to deliver that single float in an array, we'll still accept that.

Now also rebased on master

@coveralls

This comment has been minimized.

coveralls commented Apr 4, 2017

Coverage Status

Coverage increased (+0.004%) to 88.387% when pulling 3d0ddfd on arokem:non_local_means_sigma into 520b724 on nipy:master.

@coveralls

This comment has been minimized.

coveralls commented Apr 4, 2017

Coverage Status

Coverage increased (+0.004%) to 88.387% when pulling 3d0ddfd on arokem:non_local_means_sigma into 520b724 on nipy:master.

@@ -81,7 +96,7 @@ def test_nlmeans_dtype():
S0 = 200 * np.ones((20, 20, 20), dtype=np.uint16)
mask = np.zeros((20, 20, 20))
mask[10:14, 10:14, 10:14] = 1
S0n = non_local_means(S0, sigma=np.ones((20, 20, 20)), mask=mask,
S0n = non_local_means(S0, sigma=1, mask=mask,

This comment has been minimized.

@MarcCote

MarcCote Apr 7, 2017

Contributor

That probably fits on one line now.

@@ -39,7 +55,7 @@ def test_nlmeans_boundary():

S0[:10, :10, :10] = 300 + noise[:10, :10, :10]

S0n = non_local_means(S0, sigma=np.ones((20, 20, 20)) * np.std(noise),
S0n = non_local_means(S0, sigma=np.std(noise),

This comment has been minimized.

@MarcCote

MarcCote Apr 7, 2017

Contributor

That probably fits on one line now.

@arokem

This comment has been minimized.

Member

arokem commented Apr 7, 2017

@coveralls

This comment has been minimized.

coveralls commented Apr 8, 2017

Coverage Status

Coverage increased (+0.003%) to 88.386% when pulling d97a429 on arokem:non_local_means_sigma into 520b724 on nipy:master.

@MarcCote MarcCote merged commit 38d5a3d into nipy:master Apr 9, 2017

4 checks passed

codecov/patch 100% of diff hit (target 85.87%)
Details
codecov/project 85.87% (+<.01%) compared to 520b724
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
coverage/coveralls Coverage increased (+0.003%) to 88.386%
Details

ShreyasFadnavis pushed a commit to ShreyasFadnavis/dipy that referenced this pull request Sep 20, 2018

Merge pull request nipy#1166 from arokem/non_local_means_sigma
RF: Use the average sigma in the mask.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment