[ENH] Parallelize conformation #666

anibalsolon · 2017-08-14T17:39:10Z

Step t1_conform can become a bottleneck since image resampling is executed in series. However, it is possible to run it in parallel, for multiple T1w images.

This PR intends to parallelize this process by breaking it into two steps:

Decide the zoom and scale that images should take, based on all images
For each image, resample it to fit the new requirements

This process is executed before and after t1_merge, so a sub-workflow was created to wrap both steps.

This PR solves issue #613 .

effigies · 2017-08-14T17:46:07Z

Hi, thanks for this. I think if we're going to do this at this point, we should probably simplify the t1_reorient step, rather than build a workflow. Most of the machinery is entirely skipped, because t1_reorient only ever gets one image as input. I'll have a look through this in a bit; I think we can manage this with pretty minimal changes to what you've added.

anibalsolon · 2017-08-14T17:57:23Z

Hi @effigies ! Thank you for the feedback.

I believe it is possible to abstract the nb.as_closest_canonical as a Step, and put it before PruneExcessiveZoom, so the wf become:

MapNode "ClosestCanonical" (maybe better naming)
Node PruneExcessiveZoom
MapNode ConformSeries

and the t1_reorient do not use the whole wf, but just Node ClosestCanonical.

effigies · 2017-08-14T18:14:24Z

I think the ClosestCanonical node would be perfectly reasonable, but there's no need to use it before and after the template creation; we can leave nb.as_closest_canonical in the PruneExcessiveZoom and ConformSeries interfaces; it's a basically free operation in memory, so there's no need to save the result. We can just run it after t1_template.

As to the name, maybe "Reorient", "OrientRAS"? I'm not too picky here.

Also, WRT "PruneExcessiveZoom", that's only one part of what that step does (and actually the rarest, in my experience). The main purpose it serves is to calculate the template dimensions. "TemplateDimensions", "CalcTemplateSize", "TemplateParameters", or something along those lines would be better.

What do you think?

effigies

Here's a first pass review of the code. Once we decide how to go on these issues and see what the result looks like, I'll do a more fiddly, detailed review.

effigies · 2017-08-14T18:44:16Z

fmriprep/interfaces/images.py

+    target_shape = traits.List(traits.Int,
+                               desc='Target shape information')
+    target_span = traits.List(traits.Float,
+                              desc='Target span information')


I wouldn't bother with target_span, since it's just the dot-product of target_zooms and target_shape. Doing the multiplication avoids checking for the case where span != zooms * shape.

I was in doubt to parametrize it or compute the dot product on the next step, so I will now compute it. 👌

effigies · 2017-08-14T18:45:32Z

fmriprep/interfaces/images.py

+    t1w_valid_list = OutputMultiPath(exists=True, desc='valid T1w images')
+
+    target_zooms = traits.List(traits.Float,
+                               desc='Target zoom information')


Since this is a known length, you can either set a min/max length of 3, or do:

target_zooms = traits.Tuple(traits.Float, traits.Float, traits.Float, desc='...')

Perfect! I didn't know this format. Will use it now :)

effigies · 2017-08-14T18:57:22Z

fmriprep/interfaces/images.py

+
+class PruneExcessiveZoomOutputSpec(TraitedSpec):
+    t1w_dropped_list = OutputMultiPath(exists=True, desc='invalid T1w images')
+    t1w_valid_list = OutputMultiPath(exists=True, desc='valid T1w images')


The t1w_dropped_list isn't going to be used anywhere, is it? I'm not sure that this is going to be generally useful outside of fmriprep, so we don't need to add pieces we won't use. (If we move it into nipype because the people are clamoring for it, we can revisit.)

Yes! Initially, it was to be used for a posterior reporting, but I ended adding reporting in this same interface. I'll remove it for now.

effigies · 2017-08-14T19:01:09Z

fmriprep/interfaces/images.py

-
-        self._results['t1w_list'] = out_names
+        self._results['target_zooms'] = target_zooms.tolist()
+        self._results['target_shape'] = target_shape.astype(np.int64).tolist()


If we need 64 bits to describe the shape of an array, we're in trouble. This should already be an int, so I think you can skip the astype.

Hehe indeed! I cast it because it was using floats, but I will double check it, just in case. If it is not possible to remove this casting, I will switch np.int64 to int.

effigies · 2017-08-14T19:02:29Z

fmriprep/interfaces/images.py

+    t1w = File(exists=True, desc='conformed T1w image')
+
+
+class ConformSeries(SimpleInterface):


Since the series is being mapped over, perhaps "ConformImage" or possibly "Conform"?

Conform sounds good!

effigies · 2017-08-14T19:03:20Z

fmriprep/interfaces/images.py

@@ -279,6 +228,93 @@ def _run_interface(self, runtime):
        return runtime


+class ConformSeriesInputSpec(BaseInterfaceInputSpec):
+    t1w = File(exists=True, mandatory=True,


The reasons for using t1w_list are now obviated, so I'd switch to in_file, to make this more generic.

effigies · 2017-08-14T19:03:29Z

fmriprep/interfaces/images.py

+    target_shape = traits.List(traits.Int,
+                               desc='Target shape information')
+    target_span = traits.List(traits.Float,
+                              desc='Target span information')


Again, I'd drop target_span.

effigies · 2017-08-14T19:03:42Z

fmriprep/interfaces/images.py

+
+
+class ConformSeriesOutputSpec(TraitedSpec):
+    t1w = File(exists=True, desc='conformed T1w image')


out_file = File(...)

effigies · 2017-08-18T18:00:51Z

@anibalsolon No rush, but let us know if there's anything you need from us.

anibalsolon · 2017-08-19T15:06:58Z

Hi @effigies, sorry for the delay!

Thank you for the detailed review. If there is something I can improve, please let me know.

effigies

A few quick comments.

Also, have you tested this on any of your own datasets with multiple T1w images?

effigies · 2017-08-20T11:11:23Z

fmriprep/interfaces/images.py

+        while valid.any():
+            target_zooms = all_zooms[valid].min(axis=0)
+            scales = all_zooms[valid] / target_zooms
+            if np.all(scales < max_scale):


Since it's only used once, don't bother setting max_scale = self.inputs.max_scale above. Just use self.inputs.max_scale here.

effigies · 2017-08-20T17:56:25Z

fmriprep/interfaces/images.py

+                              desc='input T1w images')
+
+    max_scale = traits.Float(3.0, usedefault=True,
+                             desc='Maximum scaling factor in images to accept')


I think these can each be on a single line (max line length: 99), but I haven't checked.

In any event, we don't need a blank space between t1w_list and max_scale.

These comments apply to all of the Input/OutputSpecs.

Nice! Do you know if it is possible to enforce this pattern of attr spacing via flake?

Not that I know of.

effigies · 2017-08-20T17:58:13Z

fmriprep/interfaces/images.py

+    out_report = File(exists=True, desc='conformation report')
+
+
+CONFORMATION_TEMPLATE = """\t\t<h3 class="elem-title">Anatomical Conformation</h3>


Let's keep these above the input/output specs. So the pattern is:

CONSTANTS class XInputSpec(...): ... class XOutputSpec(...): ... class X(...): ...

effigies · 2017-08-20T19:10:44Z

fmriprep/interfaces/images.py

-    #. Orient to RAS (left-right, posterior-anterior, inferior-superior)
-    #. Along each dimension, resample to minimum voxel size, maximum number of voxels
+class TemplateDimensions(SimpleInterface):
+    """Filter a series of T1w images based on the requirements for up-sampling.


Proposed replacement for this first line, focusing on the main goal of this interface:

Finds template target dimensions for a series of T1w images, filtering low-resolution images, if necessary. Along each axis, the minimum voxel size (zoom) and the maximum number of voxels (shape) are found across images.

effigies · 2017-08-20T19:23:02Z

fmriprep/interfaces/images.py

+        else:
+            copyfile(fname, out_name, copy=True, use_hardlink=True)
+
+        self._results['out_file'] = out_name


Let's not bother copying, just point to the original file:

def _run_interface(self, runtime): # Load image, orient as RAS fname = self.inputs.in_file orig_img = nb.load(fname) reoriented = nb.as_closest_canonical(orig_img) if reoriented is not orig_img: out_name = fname_presuffix(fname, suffix='_ras', newpath=runtime.cwd) reoriented.to_filename(out_name) else: out_name = fname self._results['out_file'] = out_name

effigies · 2017-08-20T19:26:07Z

fmriprep/interfaces/images.py

+        if reoriented is not orig_img:
+            reoriented.to_filename(out_name)
+        else:
+            copyfile(fname, out_name, copy=True, use_hardlink=True)


Same thing here, wrt copying.

anibalsolon · 2017-08-20T22:07:36Z

I tested on Poldrack's MyConnectome dataset, as suggested in issue #613
http://myconnectome.org/wp/data-sharing/ Sessions 13-24

effigies

Great! Thanks. One last minor comment on the docs, but I think this is good to go. Could you go ahead and merge master? I'll run it on a couple subjects we've had issues with in the past to make sure there are no regressions.

effigies · 2017-08-21T00:04:56Z

fmriprep/interfaces/images.py

+    Performs two basic functions:
+
+    #. Orient to RAS (left-right, posterior-anterior, inferior-superior)
+    #. Along each dimension, resample to minimum voxel size, maximum number of voxels


Since this is no longer calculating the zooms, shape, we can just say:

"resample to target zooms (voxel sizes) and shape (number of voxels)"

anibalsolon · 2017-08-21T03:02:05Z

Awesome!

I cannot merge because I don't have write access.

If there are some datasets that I can help testing, just let me know.

chrisgorgo · 2017-08-21T03:32:11Z

I think @effigies meant merging "master" branch from the upstream (poldracklab/fmriprep) into your branch.

…rmation

anibalsolon · 2017-08-21T09:40:36Z

Ok! Merged with the upstream. Thanks @chrisfilo

effigies · 2017-08-21T12:21:18Z

Blocking merge on following tests (will check as I finish):

Test on ds000114 sub-02 (See ERROR: MultiRegistration::loadMovables: images have different voxel sizes #538/[RTM] ENH: Conform multiple T1w images, resampling if necessary #545)
Test on ds000114 sub-07/09 (See ERROR: talairach_afd: Talairach Transform: transforms/talairach.xfm ***FAILED*** (p=0.0000, pval=0.0000 < threshold=0.0050 #562/[RTM] FIX: Reduce interpolation artifacts in ConformSeries #564)
Test on ds000031

effigies · 2017-08-21T12:44:03Z

Good to go. Thanks a lot!

anibalsolon added 4 commits August 14, 2017 11:41

Break conformation step into 2 steps, in order to parallelize rescaling

120adea

Append output report to wf

ff63cbd

Adjust to naming conventions

0dca187

Merge branch 'master' into parallelize_conformation

e0af49c

flake accordance

6be35c9

effigies reviewed Aug 14, 2017

View reviewed changes

anibalsolon added 7 commits August 19, 2017 00:36

rename templating step

99aa720

remove target span, use tuples to zooms and shapes, remove drop list

ba16f0f

rename conform step, rename in and out parameters, compute span

57f285e

create reorientation step

63aebe8

remove workflow and use new steps

b3bf419

flake

2268fa3

do not need this cast

a7fff41

anibalsolon added 2 commits August 19, 2017 20:36

naming adjustments

e6defd3

conform to flake

30079d0

effigies reviewed Aug 20, 2017

View reviewed changes

anibalsolon added 4 commits August 20, 2017 19:28

Use parameter directly

b662e3d

Better doc

43a1254

Move constant before specs, remove spacing,

a4ad06e

Avoid copying identical files

797aea3

effigies approved these changes Aug 21, 2017

View reviewed changes

Adjust docstring

9851c2e

Merge remote-tracking branch 'upstream/master' into parallelize_confo…

095fd30

…rmation

effigies merged commit 9542c4f into nipreps:master Aug 21, 2017

effigies mentioned this pull request Sep 20, 2017

MNI reslicing failure #703

Closed

		t1w = File(exists=True, desc='conformed T1w image')


		class ConformSeries(SimpleInterface):



		class ConformSeriesOutputSpec(TraitedSpec):
		t1w = File(exists=True, desc='conformed T1w image')

		out_report = File(exists=True, desc='conformation report')


		CONFORMATION_TEMPLATE = """\t\t<h3 class="elem-title">Anatomical Conformation</h3>

[ENH] Parallelize conformation #666

[ENH] Parallelize conformation #666

Conversation

anibalsolon commented Aug 14, 2017

effigies commented Aug 14, 2017

anibalsolon commented Aug 14, 2017

effigies commented Aug 14, 2017

effigies left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

effigies commented Aug 18, 2017

anibalsolon commented Aug 19, 2017 • edited Loading

effigies left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

anibalsolon commented Aug 20, 2017

effigies left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

anibalsolon commented Aug 21, 2017

chrisgorgo commented Aug 21, 2017

anibalsolon commented Aug 21, 2017

effigies commented Aug 21, 2017 • edited Loading

effigies commented Aug 21, 2017

anibalsolon commented Aug 19, 2017 •

edited

Loading

effigies commented Aug 21, 2017 •

edited

Loading