[ENH] Add parallelization to gibbs denoising

[j1c]

Added parallelization to gibbs denoising. I used the utility class MapWrapper from scipy for parallelization, which really just a wrapper for Pool in multiprocessing module with some convenient functionality for dealing with number of workers, etc. Let me know if this is acceptable.

Main thing I changed is how the axis of the input data are ordered within the gibbs_denoise function since its much easier to work with the array as an iterable if the axis we are iterating is the first one, not the last one. Some lines had to be changed to accommodate for this change in axis ordering.

Closes #2236

[pep8speaks]

Hello @j1c, Thank you for updating !

Cheers ! There are no PEP8 issues in this Pull Request. 🍻

Comment last updated at 2020-10-10 02:55:36 UTC

[codecov]

Codecov Report

Merging #2250 into master will increase coverage by 1.98%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##           master    #2250      +/-   ##
==========================================
+ Coverage   89.38%   91.36%   +1.98%     
==========================================
  Files         252      252              
  Lines       32941    32972      +31     
  Branches     3465     3469       +4     
==========================================
+ Hits        29444    30126     +682     
+ Misses       2766     2085     -681     
- Partials      731      761      +30     

Impacted Files	Coverage Δ
dipy/denoise/gibbs.py	100.00% <100.00%> (ø)
dipy/denoise/tests/test_gibbs.py	100.00% <100.00%> (ø)
dipy/workflows/denoise.py	88.73% <100.00%> (ø)
dipy/io/utils.py	86.41% <0.00%> (+0.54%)	⬆️
dipy/workflows/stats.py	85.81% <0.00%> (+0.70%)	⬆️
dipy/nn/model.py	92.59% <0.00%> (+1.85%)	⬆️
dipy/nn/tests/test_tf.py	88.46% <0.00%> (+1.92%)	⬆️
dipy/io/streamline.py	81.48% <0.00%> (+3.70%)	⬆️
dipy/io/tests/test_stateful_tractogram.py	93.20% <0.00%> (+4.07%)	⬆️
dipy/utils/optpkg.py	78.26% <0.00%> (+4.34%)	⬆️
... and 8 more

[j1c]

Ok I see that MapWrapper wont work with the scipy version in travis so I'll work on changing to using Pool

[skoudoro]

Nice! Thank you again @j1c for doing that!

Do you have a performance comparison before and after this update? just to get an idea.
Overall, it looks good to me, so +1 for merging this. But I just need to try your PR.

[skoudoro]

Also, Can you add this option to our command-line interface: https://github.com/dipy/dipy/blob/master/dipy/workflows/denoise.py#L244. Thanks!

[grlee77]

Do you have a performance comparison before and after this update? just to get an idea.

I ran it locally on a 10-core Intel Skylake-X system and it seems the acceleration provided is equal to the number of physical cores! With num_threads=0, 20 threads get used due to hyperthreading and the time to run the following line in denoise_gibbs.py was reduced from 107 seconds to 10.7 seconds.

dipy/doc/examples/denoise_gibbs.py

Line 171 in 1dc577b

data_corrected = gibbs_removal(data_slices, slice_axis=2)

[skoudoro]

That's great news! Thank you @grlee77 for the benchmark test. So After the update, it should be ready to go.

[grlee77]

Can you also update doc/examples/denoise_gibbs.py as follows:

The following line needs inplace=False or the plot shows no difference (this problem in the demo was introduced in #2239, but we may as well fix it here)

dipy/doc/examples/denoise_gibbs.py

Line 82 in ace9c7a

t1_unring = gibbs_removal(t1_gibbs)

Add num_threads=None to the 3D call at the following line so the demo completes more quickly:

dipy/doc/examples/denoise_gibbs.py

Line 171 in ace9c7a

data_corrected = gibbs_removal(data_slices, slice_axis=2)

[grlee77]

Further reduction from 10.7 seconds to 6.9 seconds was observed when the proposed changes from #2253 are applied on top of this PR.

[skoudoro]

All comments have been addressed.

I will wait until tomorrow for any additional comments otherwise it is ready to go. Thanks @j1c!

[skoudoro]

No additional comment so here we go! Thanks @j1c! merging