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

Optimize dipy.align.reslice #757

Merged
merged 6 commits into from Nov 5, 2015

Conversation

Projects
None yet
3 participants
@dimrozakis
Contributor

dimrozakis commented Oct 31, 2015

  • Using a 1D transformation matrix instead of a 2D diagonal matrix reduces computation time.
  • Using the output argument in affine_transform reduces memory usage.

dimrozakis added some commits Oct 31, 2015

Optimize dipy.align.reslice by using 1D matrix
Pass a 1D tranformation matrix to scipy.ndimage.affine_transform instead
of a 2D diagonal matrix. Then affine_transform uses a more efficient
algorithm that reduces completion time at around 50%.
@arokem

This comment has been minimized.

Member

arokem commented Oct 31, 2015

Looks great to me. If someone else could take a look that would be great. Otherwise, if no one has any comments, I can merge this towards the end of the week. Thanks!

Add multiprocessing support in dipy.align.reslice
Add the  option to use multiprocessing.Pool in dipy.align.reslice with
configurable pool size. Applicable only when using a 4D data array, in
which case the list of affine transformations for each index of the 4th
axis is submitted as a distinct job to the worker pool.
@dimrozakis

This comment has been minimized.

Contributor

dimrozakis commented Nov 1, 2015

In advantis-io@7f7e8a7 I added the option to use multiprocessing.Pool in reslice. Should I push the commit into this branch as well?

@Garyfallidis

This comment has been minimized.

Member

Garyfallidis commented Nov 1, 2015

Good idea @dimrozakis. Use num_processes as a variable name rather than mp. We usually use num_processes or nbr_processes when using multiprocessing and num_threads when using multithreading.

@arokem

This comment has been minimized.

arokem commented on dipy/align/reslice.py in e9ef047 Nov 5, 2015

Did not notice this before: for this to render properly, there needs to be a space between the variable name and the colon:

num_processes : int

Sphinx is very finnicky!

@dimrozakis

This comment has been minimized.

Contributor

dimrozakis commented Nov 5, 2015

@arokem I just fixed the missing space issue in the num_processes definition of dipy.align.reslice docstring according to your comment

@arokem

This comment has been minimized.

Member

arokem commented Nov 5, 2015

Thanks! I will wait for travis to give this a run through, and then merge...

On Wed, Nov 4, 2015 at 5:41 PM, Dimitris Rozakis notifications@github.com
wrote:

@arokem https://github.com/arokem I just fixed the missing space issue
in the num_processes definition of dipy.align.reslice docstring according
to your comment


Reply to this email directly or view it on GitHub
#757 (comment).

@arokem

This comment has been minimized.

Member

arokem commented Nov 5, 2015

I just noticed that test coverage here seems rather spotty (63% for the module). Any chance you could do something about that? Thanks!

@dimrozakis

This comment has been minimized.

Contributor

dimrozakis commented Nov 5, 2015

@arokem I added more tests, now coverage is at 98%

The 8th test case, the one with python-vtk fails. I have no idea why. It failed on the previous commit as well, which simply added a space character, so it must involve something merged to master since this branch was forked?

@arokem

This comment has been minimized.

Member

arokem commented Nov 5, 2015

Thanks. That's fantastic!

The VTK machine failure is a known (and rather hard!) issue. We'll sort it
out, but maybe not on this PR...

On Wed, Nov 4, 2015 at 7:49 PM, Dimitris Rozakis notifications@github.com
wrote:

@arokem https://github.com/arokem I added more tests, now coverage is
at 98%

The 8th test case, the one with python-vtk fails. I have no idea why. It
failed on the previous commit as well, which simply added a space
character, so it must involve something merged to master since this branch
was forked?


Reply to this email directly or view it on GitHub
#757 (comment).

arokem added a commit that referenced this pull request Nov 5, 2015

Merge pull request #757 from tomotech/reslice
Optimize dipy.align.reslice

@arokem arokem merged commit 44b5f1a into nipy:master Nov 5, 2015

1 check failed

continuous-integration/travis-ci/pr The Travis CI build failed
Details

@dimrozakis dimrozakis deleted the advantis-io:reslice branch Nov 7, 2015

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