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

arokem · 2016-12-17T00:40:50Z

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

Addresses #1131

arokem · 2016-12-17T01:50:31Z

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 · 2017-01-07T23:18:09Z

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 · 2017-01-07T23:35:16Z

Codecov Report

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

@@            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 · 2017-01-07T23:39:48Z

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 · 2017-01-07T23:43:15Z

I liked it. 👍

arokem · 2017-01-07T23:44:43Z

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

coveralls · 2017-01-08T03:48:38Z

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

arokem · 2017-01-24T15:19:36Z

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

arokem · 2017-02-03T14:23:19Z

Anyone want to take a look at this?

MarcCote · 2017-02-09T15:23:30Z

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

samuelstjean · 2017-02-09T15:50:56Z

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 · 2017-03-14T21:30:29Z

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 · 2017-03-15T01:46:26Z

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

arokem · 2017-03-23T14:11:51Z

Now rebased on master

coveralls · 2017-03-23T17:26:15Z

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

MarcCote · 2017-04-03T02:08:49Z

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

samuelstjean · 2017-04-03T06:23:41Z

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 · 2017-04-03T12:36:38Z

Good comments both -- thanks for taking a look. I've removed the nanmean cruft and added a check up-front for scalar sigma.

…

On Sun, Apr 2, 2017 at 11:23 PM, Samuel St-Jean ***@***.***> wrote: - 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 — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#1166 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAHPNq2H5mKbsUisCZS1DWKiWDf6mGUyks5rsJBugaJpZM4LPuI8> .

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

arokem · 2017-04-04T14:33:09Z

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 · 2017-04-04T15:35:46Z

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

coveralls · 2017-04-04T16:33:35Z

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

MarcCote · 2017-04-07T23:38:46Z

dipy/denoise/tests/test_non_local_means.py

@@ -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,


That probably fits on one line now.

MarcCote · 2017-04-07T23:39:23Z

dipy/denoise/tests/test_non_local_means.py

@@ -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),


That probably fits on one line now.

arokem · 2017-04-07T23:43:26Z

Indeed. I've removed some other un-needed whitespace, while I was at it.

…

On Fri, Apr 7, 2017 at 4:39 PM, Marc-Alexandre Côté < ***@***.***> wrote: ***@***.**** commented on this pull request. ------------------------------ In dipy/denoise/tests/test_non_local_means.py <#1166 (comment)>: > @@ -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), That probably fits on one line now. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#1166 (review)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAHPNsx9GfZcrVjVKmECDybI2_HNZahyks5rtskrgaJpZM4LPuI8> .

coveralls · 2017-04-08T00:41:21Z

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

RF: Use the average sigma in the mask.

arokem force-pushed the non_local_means_sigma branch from bb0861a to 48ab1c4 Compare January 7, 2017 23:39

arokem force-pushed the non_local_means_sigma branch 3 times, most recently from 3424c19 to 40149a5 Compare January 8, 2017 02:09

arokem mentioned this pull request Jan 14, 2017

Time for a new release - scipy 0.18? #1169

Closed

arokem force-pushed the non_local_means_sigma branch from 204f94f to d321414 Compare March 23, 2017 14:11

MarcCote approved these changes Apr 3, 2017

View reviewed changes

arokem added 5 commits April 4, 2017 07:31

RF: Use the average sigma in the mask.

623daf3

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

BF: If numpy version doesn't have nanmean, roll our own.

539f508

RF: Accept only scalar sigma as input.

a91591b

Accomodate API changes in other tests.

58fb9e4

Remove cruft, add check for scalar sigma.

0d41fc3

Allow arrays, if they are singletons.

3d0ddfd

arokem force-pushed the non_local_means_sigma branch from f5ea84d to 3d0ddfd Compare April 4, 2017 14:31

MarcCote reviewed Apr 7, 2017

View reviewed changes

Eliminate un-needed whitespace.

d97a429

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

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

Merge pull request dipy#1166 from arokem/non_local_means_sigma

1050bb6

RF: Use the average sigma in the mask.

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

arokem commented Dec 17, 2016

arokem commented Dec 17, 2016

MarcCote commented Jan 7, 2017

codecov-io commented Jan 7, 2017 •

edited

arokem commented Jan 7, 2017

MarcCote commented Jan 7, 2017

arokem commented Jan 7, 2017

coveralls commented Jan 8, 2017

arokem commented Jan 24, 2017

arokem commented Feb 3, 2017

MarcCote commented Feb 9, 2017

samuelstjean commented Feb 9, 2017

arokem commented Mar 14, 2017

coveralls commented Mar 15, 2017

arokem commented Mar 23, 2017

coveralls commented Mar 23, 2017

MarcCote commented Apr 3, 2017

samuelstjean commented Apr 3, 2017

arokem commented Apr 3, 2017 via email

arokem commented Apr 4, 2017

coveralls commented Apr 4, 2017

coveralls commented Apr 4, 2017

MarcCote Apr 7, 2017

MarcCote Apr 7, 2017

arokem commented Apr 7, 2017 via email

coveralls commented Apr 8, 2017

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

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

Conversation

arokem commented Dec 17, 2016

arokem commented Dec 17, 2016

MarcCote commented Jan 7, 2017

codecov-io commented Jan 7, 2017 • edited

Codecov Report

arokem commented Jan 7, 2017

MarcCote commented Jan 7, 2017

arokem commented Jan 7, 2017

coveralls commented Jan 8, 2017

arokem commented Jan 24, 2017

arokem commented Feb 3, 2017

MarcCote commented Feb 9, 2017

samuelstjean commented Feb 9, 2017

arokem commented Mar 14, 2017

coveralls commented Mar 15, 2017

arokem commented Mar 23, 2017

coveralls commented Mar 23, 2017

MarcCote commented Apr 3, 2017

samuelstjean commented Apr 3, 2017

arokem commented Apr 3, 2017 via email

arokem commented Apr 4, 2017

coveralls commented Apr 4, 2017

coveralls commented Apr 4, 2017

MarcCote Apr 7, 2017

Choose a reason for hiding this comment

MarcCote Apr 7, 2017

Choose a reason for hiding this comment

arokem commented Apr 7, 2017 via email

coveralls commented Apr 8, 2017

codecov-io commented Jan 7, 2017 •

edited