-
Notifications
You must be signed in to change notification settings - Fork 437
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
[ENH] Add parallelization to gibbs denoising #2250
Conversation
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 Report
@@ 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
|
Ok I see that |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Also, Can you add this option to our command-line interface: https://github.com/dipy/dipy/blob/master/dipy/workflows/denoise.py#L244. Thanks! |
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 dipy/doc/examples/denoise_gibbs.py Line 171 in 1dc577b
|
That's great news! Thank you @grlee77 for the benchmark test. So After the update, it should be ready to go. |
Can you also update The following line needs dipy/doc/examples/denoise_gibbs.py Line 82 in ace9c7a
Add dipy/doc/examples/denoise_gibbs.py Line 171 in ace9c7a
|
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. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
All comments have been addressed.
I will wait until tomorrow for any additional comments otherwise it is ready to go. Thanks @j1c!
No additional comment so here we go! Thanks @j1c! merging |
Added parallelization to gibbs denoising. I used the utility class
MapWrapper
from scipy for parallelization, which really just a wrapper forPool
inmultiprocessing
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