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

Restored the older implementation of nlmeans #1088

Merged
merged 3 commits into from Jun 24, 2016

Conversation

Projects
None yet
6 participants
@riddhishb
Contributor

riddhishb commented Jun 24, 2016

The changes proposed in #1018 for the nlmeans function are now omitted, after the discussion in #1083. This results in still having the corner case mentioned in #1011
@Garyfallidis Do have a look.

@@ -108,9 +103,10 @@ def _nlmeans_3d(double[:, :, ::1] arr, double[:, :, ::1] mask,
# move the block
with nogil, parallel():
for i in prange(B + P, I - B - P, schedule="dynamic"):

This comment has been minimized.

@arokem

arokem Jun 24, 2016

Member

I believe that you can retain this change (introduced here: 4ee6d49) without issue. As I understand it, this only (substantially!) speeds up performance without change in the results. Worth making sure, though.

This comment has been minimized.

@riddhishb

riddhishb Jun 24, 2016

Contributor

Yes looking at it. compared speeds, the schedule = "dynamic" makes a difference!

This comment has been minimized.

@arokem

arokem Jun 24, 2016

Member

And results are identical?
On Jun 24, 2016 9:03 AM, "Riddhish Bhalodia" notifications@github.com
wrote:

In dipy/denoise/denspeed.pyx
#1088 (comment):

@@ -108,9 +103,10 @@ def _nlmeans_3d(double[:, :, ::1] arr, double[:, :, ::1] mask,

 # move the block
 with nogil, parallel():
  •    for i in prange(B + P, I - B - P, schedule="dynamic"):
    

Yes looking at it. compared speeds, the schedule = "dynamic" makes a
difference!


You are receiving this because you commented.
Reply to this email directly, view it on GitHub
https://github.com/nipy/dipy/pull/1088/files/c6068c3877c0dd6a2a7c15dac27ebe21c50a3d9c#r68420578,
or mute the thread
https://github.com/notifications/unsubscribe/AAHPNqdFtD0XTBIGwODNvA84dFBwfpd2ks5qO_--gaJpZM4I95WC
.

This comment has been minimized.

@riddhishb

riddhishb Jun 24, 2016

Contributor

Yeah identical, the speed improvements are from 4.4 seconds to 3.7 seconds with 4 threads

W = <double *> malloc(PS * PS * PS * sizeof(double))
cache = <double *> malloc(TS * TS * TS * sizeof(double))
sigma_block = <double *> malloc(TS * TS * TS * sizeof(double))
W = <double * > malloc(BS * BS * BS * sizeof(double))

This comment has been minimized.

@Garyfallidis

Garyfallidis Jun 24, 2016

Member

No need for space after *. Correct everywhere please.

@@ -307,7 +283,7 @@ cdef cnp.npy_intp copy_block_3d(double * dest,
for i in range(I):
for j in range(J):
memcpy(&dest[i * J * K + j * K], &source[i + min_i, j + min_j, min_k], K * sizeof(double))
memcpy(& dest[i * J * K + j * K], & source[i + min_i, j + min_j, min_k], K * sizeof(double))

This comment has been minimized.

@Garyfallidis

Garyfallidis Jun 24, 2016

Member

No need for space after &. I know this is a bit puzzling. Because those operators are from C in a way you can consider them as part of the variable name. Rather than an operator.

@coveralls

This comment has been minimized.

coveralls commented Jun 24, 2016

Coverage Status

Coverage remained the same at 82.88% when pulling 817e1b5 on riddhishb:nlmeans_restore into 2836d60 on nipy:master.

@codecov-io

This comment has been minimized.

codecov-io commented Jun 24, 2016

Current coverage is 80.81%

Merging #1088 into master will not change coverage

@@             master      #1088   diff @@
==========================================
  Files           200        200          
  Lines         22985      22985          
  Methods           0          0          
  Messages          0          0          
  Branches       2309       2309          
==========================================
  Hits          18576      18576          
  Misses         3933       3933          
  Partials        476        476          

Powered by Codecov. Last updated by 2836d60...817e1b5

@Garyfallidis

This comment has been minimized.

Member

Garyfallidis commented Jun 24, 2016

Thank you @riddhishb.

@mdesco this PR restores the older version of nlmeans. To make things clearer @riddhishb will create a new function named non_local_means that will have the blockwise implementation which from what we see it is better than the voxelwise implementation (quantitatively and qualitatively). The new function will be added with the adaptive non local means PR. When that happens (which is very soon) a warning will be added to the initial nlmeans to say that this function has now a better alternative (i.e. the blockwise version) so you should start updating your scripts. But the initial implemenation will stay around for some time so that the transition is smoother.

Thanks again to @riddhishb for all the excellent work. Keep it up!

@Garyfallidis Garyfallidis merged commit 220d883 into nipy:master Jun 24, 2016

3 of 4 checks passed

continuous-integration/travis-ci/pr The Travis CI build is in progress
Details
codecov/patch Coverage not affected when comparing 2836d60...817e1b5
Details
codecov/project 80.81% (+0.00%) compared to 2836d60
Details
coverage/coveralls Coverage remained the same at 82.88%
Details
@Garyfallidis

This comment has been minimized.

Member

Garyfallidis commented Jun 24, 2016

@mdesco please check if what is in master works for you now. Thanks again for reporting this quickly.

@mdesco

This comment has been minimized.

Contributor

mdesco commented Jun 26, 2016

Things seem back to normal according to my simple tests and scripts. Thank you!

@arokem arokem referenced this pull request Sep 16, 2016

Closed

nlmeans problem #1083

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