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

Parallelize diffeomorphic registration #1613

Open
wants to merge 7 commits into
base: master
from

Conversation

Projects
None yet
4 participants
@RicciWoo
Copy link
Contributor

RicciWoo commented Aug 10, 2018

Parallelize diffeomorphic registration using prange and local function:

  1. parallelized _compose_vector_fields_3d in vector_fields.pyx;
  2. parallelized warp_3d in vector_fields.pyx.

To specify number of threads: add num_threads to SymmetricDiffeomorphicRegistration

sdr = SymmetricDiffeomorphicRegistration(metric, level_iters, num_threads=4)

@codecov-io

This comment has been minimized.

Copy link

codecov-io commented Aug 10, 2018

Codecov Report

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

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #1613      +/-   ##
==========================================
+ Coverage   87.34%   87.34%   +<.01%     
==========================================
  Files         246      246              
  Lines       31811    31822      +11     
  Branches     3451     3454       +3     
==========================================
+ Hits        27785    27795      +10     
  Misses       3204     3204              
- Partials      822      823       +1
Impacted Files Coverage Δ
dipy/align/imwarp.py 98.43% <100%> (+0.03%) ⬆️
dipy/core/graph.py 73.8% <0%> (-1.2%) ⬇️

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 5a6aa5a...20a0b19. Read the comment docs.

RicciWoo added some commits Aug 11, 2018

@RicciWoo RicciWoo force-pushed the RicciWoo:syn-par-comp-warp branch from 30997a1 to 4f79f77 Aug 12, 2018

@Borda
Copy link
Contributor

Borda left a comment

valuable change but some code enhancement would be welcome

@@ -382,8 +386,13 @@ def _warp_forward(self, image, interpolation='linear',

warp_f = self._get_warping_function(interpolation)

warped = warp_f(image, self.forward, affine_idx_in, affine_idx_out,
affine_disp, out_shape)
if self.dim == 2:

This comment has been minimized.

@Borda

Borda Jan 17, 2019

Contributor

what about rather set the setting the nb of threads

num_threads = self.num_threads if self.dim <= 2 else 1
warped = warp_f(image, self.forward, affine_idx_in, affine_idx_out, affine_disp, out_shape, num_threads)
@@ -493,8 +502,12 @@ def _warp_backward(self, image, interpolation='linear',

warp_f = self._get_warping_function(interpolation)

warped = warp_f(image, self.backward, affine_idx_in, affine_idx_out,
affine_disp, out_shape)
if self.dim == 2:

This comment has been minimized.

@Borda

Borda Jan 17, 2019

Contributor

similar as above...

@@ -920,6 +934,9 @@ def __init__(self,
inv_tol : float
the displacement field inversion algorithm will stop iterating
when the inversion error falls below this threshold
num_threads : int

This comment has been minimized.

@Borda

Borda Jan 17, 2019

Contributor

I would rater follow the sklearn syntax, using -1 for default/all threads used instead introducing None

current_disp_spacing,
self.inv_iter, self.inv_tol,
self.moving_to_ref.forward))
if self.dim == 2:

This comment has been minimized.

@Borda

Borda Jan 17, 2019

Contributor

setting just the right number of threads would avoid doubling the code length

num_threads = self.num_threads if self.dim <= 2 else 1
...
for j in range(nc1):

# This is the only place we access d1[k, i, j]
dkk = d1[k, i, j, 0]

This comment has been minimized.

@Borda

Borda Jan 17, 2019

Contributor

the name convention dkk or diii is very unintuitive

for i in range(nrows):
for j in range(ncols):
if affine_idx_in is None:
dkk = d1[k, i, j, 0]

This comment has been minimized.

@Borda

Borda Jan 17, 2019

Contributor

introducing one more for cycle instead repetitive using triple commands would make the code much more readable

for i in range(nr1):
for j in range(nc1):

# This is the only place we access d1[k, i, j]

This comment has been minimized.

@Borda

Borda Jan 17, 2019

Contributor

introducing one more for cycle instead repetitive using triple commands would make the code much more readable

dii = d1[k, i, j, 1]
djj = d1[k, i, j, 2]
else:
dk = _apply_affine_3d_x0(

This comment has been minimized.

@Borda

Borda Jan 17, 2019

Contributor

is line break really needed?

@@ -1896,6 +2032,9 @@ def warp_3d(floating[:, :, :] volume, floating[:, :, :, :] d1,
the matrix C in eq. (1) above
out_shape : array, shape (3,)
the number of slices, rows and columns of the sampling grid
num_threads : int

This comment has been minimized.

@Borda

Borda Jan 17, 2019

Contributor

rather use -1 for default, see sklearn


for i in range(nrows):
for j in range(ncols):
if affine_idx_in is None:

This comment has been minimized.

@Borda

Borda Jan 17, 2019

Contributor

similar as above, simplify the code using one more inter loop

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