WIP: Adding a python wrapper for ICA_AROMA #1993

jdkent · 2017-05-03T05:15:25Z

This is my attempt at adding ICA_AROMA's functionality to nipype, although it would probably be better just to incorporate the code from ICA_AROMA.py, I didn't want to take the code and then have it be maintained in two places, so I wrote a wrapper treating the python file as a commandline tool. I don't know the best place for commandline tools, but ICA_AROMA.py uses fsl tools so I placed it there.

Thanks for looking!

keeping up to date

chrisgorgo · 2017-05-03T05:19:17Z

+1 for calling AROMA instead of copying the code

effigies

Thanks for this. I've marked up a few changes. There are some issues that need fixing, but others are aesthetic, so feel free to argue points.

A few of them are coding style changes. Consider using flake8 to get style suggestions. That should reduce the amount of review space that needs to be spent on some, and may help catch some bugs before committing.

effigies · 2017-05-03T16:29:04Z

nipype/interfaces/fsl/ICA_AROMA.py

+                        desc='If a feat directory exists and temporal filtering '
+                        'has not been run yet, ICA_AROMA can use the files in '
+                        'this directory.',mandatory=False,xor=['infile','mask','affmat','warp','mc'])
+    infile = File(exists=True, 


in_file would be more consistent with other interfaces.

adapted this change into updated script

effigies · 2017-05-03T16:41:24Z

nipype/interfaces/fsl/ICA_AROMA.py

+import os
+
+class ICA_AROMAInputSpec(CommandLineInputSpec):
+    featDir = Directory(exists=True,


There's a mix of camelCase and snake_case in your code. I think the dominant choice in nipype is snake, so I'd consider changing featDir to feat_dir.

I believe this should have argstr='-feat %s'.

Also, just for (my own sense of) readability, I'd put mandatory=False on the same line as exists=True. And I prefer to put desc last, just because it often is multi-line. These suggestions are by no means a matter of consensus; there are several styles for input specs, so don't feel obligated to indulge me here.

I haven't heard of flake8, that looks really useful, thanks!

argstr: D'oh, I made a couple of errors where I didn't specify the string correctly

I'll move 'desc' to the end and move 'mandatory' next to 'exists'. Question: if mandatory=False is the default behavior, should I still be explicit or is that too wordy?

If it's non-mandatory, don't include it. But since you're in the situation where you have to either have feat_dir or in_file, then both should be mandatory, but xor one another. (The mandatory check is smart enough to figure out these required-but-mutually-exclusive cases.)

I think your motion correction params, affine transform and warp are all required (if not using feat_dir), so they should be set to mandatory as well, but the mask is generated by the program if not supplied, so it doesn't need to be set to mandatory.

effigies · 2017-05-03T16:41:44Z

nipype/interfaces/fsl/ICA_AROMA.py

+                        'this directory.',mandatory=False,xor=['infile','mask','affmat','warp','mc'])
+    infile = File(exists=True, 
+                  desc='volume to be denoised', 
+                  argstr='-i %s',mandatory=False,xor=['featDir'])


Space after commas. (Putting this here to stand in for all lines with no spaces after commas.)

done (I think).

effigies · 2017-05-03T16:49:16Z

nipype/interfaces/fsl/ICA_AROMA.py

+                  desc='volume to be denoised', 
+                  argstr='-i %s',mandatory=False,xor=['featDir'])
+    outdir = Directory(desc='path to output directory', 
+                  argstr='-o %s',mandatory=True)


I'd suggest making this 'out' by default, and not requiring users to input it. Typically in nipype you don't really care about specifying locations during processing. Each Node gets a working directory where all of the outputs can be placed. So this would be:

out_dir = Directory('out', argstr='-o %s', desc='output directory')

I'm adding the underscore for consistency with some other interfaces.

your comment here helped me understand how nipype works a bit better, thanks!

effigies · 2017-05-03T16:50:02Z

nipype/interfaces/fsl/ICA_AROMA.py

+                  argstr='-o %s',mandatory=True)
+    mask = File(exists=True, 
+                desc='path/name volume mask', 
+                argstr='-m %s',mandatory=False,xor=['featDir'])


mandatory=False is implied, so you can remove it here.

effigies · 2017-05-03T17:30:38Z

nipype/interfaces/fsl/ICA_AROMA.py

+    >>> AROMA_obj.inputs.mc=mc
+    >>> AROMA_obj.inputs.mask=mask
+    >>> AROMA_obj.inputs.denoise_type=denoise_type
+    >>> AROMA_obj.inputs.outdir=outDir


I wouldn't separate AROMA_obj.inputs.x = y from y = 'somefile'. It's extra lines without much value.

Also, these files need to exist. You could create a set of empty files in nipype/testing/data/feat_dir or something like that, and then use:

>>> AROMA_obj.inputs.in_file = 'feat_dir/mcImg_brain.nii.gz'

And so on.

effigies · 2017-05-03T17:32:13Z

nipype/interfaces/fsl/ICA_AROMA.py

+    -mc /path/to/mcImg.par 
+    -o /path/to/ICA_AROMA_testout 
+    -warp /path/to/T1toMNI_warp.nii.gz'
+    >>> AROMA_obj.run()


Don't actually run this during the doctests. Do

>>> AROMA_obj.run() # doctest: +SKIP

effigies · 2017-05-03T17:33:01Z

nipype/interfaces/fsl/ICA_AROMA.py

+          print "No denoising selected"
+        else:
+          raise RuntimeError('denoise_type must be specified as one of'
+                             ' noaggr,aggr,both, or none')


This particular exception will be caught by the Enum trait, above.

effigies · 2017-05-03T17:33:28Z

nipype/interfaces/fsl/ICA_AROMA.py

+        elif denoising_strategy is "both":
+          outputs['out_file'] = (os.path.join(outdir,'denoised_func_data_nonaggr.nii.gz'), os.path.join(outdir,'denoised_func_data_aggr.nii.gz'))
+        elif denoising_strategy is "none":
+          print "No denoising selected"


We support Python 3, so if you're going to print, use print().

effigies · 2017-05-03T17:36:09Z

nipype/interfaces/fsl/ICA_AROMA.py

+        elif denoising_strategy is "aggr":
+          outputs['out_file'] = os.path.join(outdir,'denoised_func_data_aggr.nii.gz')
+        elif denoising_strategy is "both":
+          outputs['out_file'] = (os.path.join(outdir,'denoised_func_data_nonaggr.nii.gz'), os.path.join(outdir,'denoised_func_data_aggr.nii.gz'))


Assuming you split the output like I suggest above, you could do the following:

out_dir = os.path.abspath(self.inputs.out_dir) if self.inputs.denoise_type in ('aggr', 'both'): outputs['aggr_denoised_file'] = os.path.join(out_dir, 'denoised_func_data_aggr.nii.gz') if self.inputs.denoise_type in ('nonaggr', 'both'): outputs['nonaggr_denoised_file'] = os.path.join(out_dir, 'denoised_func_data_nonaggr.nii.gz')

keeping up to date

codecov-io · 2017-05-03T22:19:24Z

Codecov Report

Merging #1993 into master will decrease coverage by 19.67%.
The diff coverage is n/a.

@@             Coverage Diff             @@
##           master    #1993       +/-   ##
===========================================
- Coverage   72.15%   52.48%   -19.68%     
===========================================
  Files        1117      118      -999     
  Lines       56456    23565    -32891     
  Branches     8112        0     -8112     
===========================================
- Hits        40736    12368    -28368     
+ Misses      14443    11197     -3246     
+ Partials     1277        0     -1277

Flag	Coverage Δ
#smoketests	`52.48% <ø> (-19.68%)`	⬇️
#unittests	`?`

Impacted Files	Coverage Δ
nipype/pipeline/plugins/callback_log.py	`12% <0%> (-80%)`	⬇️
nipype/workflows/smri/freesurfer/ba_maps.py	`11.11% <0%> (-79.17%)`	⬇️
nipype/workflows/rsfmri/fsl/resting.py	`14.51% <0%> (-70.97%)`	⬇️
nipype/workflows/smri/freesurfer/autorecon2.py	`2.3% <0%> (-68.85%)`	⬇️
nipype/workflows/smri/freesurfer/autorecon3.py	`2.09% <0%> (-66.9%)`	⬇️
nipype/pipeline/plugins/pbsgraph.py	`29.41% <0%> (-64.71%)`	⬇️
nipype/workflows/smri/freesurfer/bem.py	`38.46% <0%> (-61.54%)`	⬇️
nipype/interfaces/utility/csv.py	`42.85% <0%> (-57.15%)`	⬇️
nipype/workflows/smri/freesurfer/recon.py	`8.57% <0%> (-56.43%)`	⬇️
nipype/algorithms/confounds.py	`31.69% <0%> (-46.25%)`	⬇️
... and 1053 more

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 f39e222...261fade. Read the comment docs.

jdkent · 2017-05-03T22:36:12Z

@effigies : I believe I responded to all of your edits, thanks for the detailed feedback and suggestions, it made it that much easier to change the code.

Comments addressed

effigies · 2017-05-03T22:38:36Z

@jdkent Thanks, it looks much improved! I'll have a more detailed look after the tests run. (Possibly in the morning. There's a bit of a test backlog.)

effigies

This is looking great! A couple more comments and I think it'll be ready to go.

You should also run make specs from the repository root directory, and then add nipype/interfaces/fsl/tests/test_auto_ICA_AROMA.py. (Don't add all of the changes; just the ones relevant to your PR.)

Once you've committed your changes, you can clean up by doing something like:

git add . && git stash && git stash drop

effigies · 2017-05-04T17:27:30Z

nipype/interfaces/fsl/ICA_AROMA.py

+    >>> AROMA_obj.inputs.denoise_type='both'
+    >>> AROMA_obj.inputs.out_dir='ICA_testout'
+    >>> AROMA_obj.cmdline
+    u'ICA_AROMA.py -den both -warp /home/travis/build/nipy/nipype/nipype/testing/data/warpfield.nii -i /home/travis/build/nipy/nipype/nipype/testing/data/functional.nii -m /home/travis/build/nipy/nipype/nipype/testing/data/mask.nii.gz -affmat /home/travis/build/nipy/nipype/nipype/testing/data/func_to_struct.mat -mc /home/travis/build/nipy/nipype/nipype/testing/data/functional.par -o ICA_testout'


You don't need example_data(), which adds the full path, for doctests.

How about this:

>>> AROMA_obj.inputs.in_file = 'functional.nii' >>> AROMA_obj.inputs.mat_file = 'func_to_struct.mat' >>> AROMA_obj.inputs.fnirt_warp_file = 'warpfield.nii' >>> AROMA_obj.inputs.motion_parameters = 'fsl_mcflirt_movpar.txt' >>> AROMA_obj.inputs.mask = 'mask.nii.gz' >>> AROMA_obj.inputs.denoise_type = 'both' >>> AROMA_obj.inputs.out_dir='ICA_testout' >>> AROMA_obj.cmdline # doctest: +ALLOW_UNICODE 'ICA_AROMA.py -den both -warp warpfield.nii -i functional.nii -m mask.nii.gz \ -affmat func_to_struct.mat -mc fsl_mcflirt_movpar.txt -o ICA_testout'

thanks for this!

effigies · 2017-05-04T17:28:06Z

nipype/interfaces/fsl/ICA_AROMA.py

+    output_spec = ICA_AROMAOutputSpec
+
+    def _list_outputs(self):
+


No blank line here.

effigies · 2017-05-04T17:28:15Z

nipype/interfaces/fsl/ICA_AROMA.py

+        outputs['out_dir'] = out_dir
+        #outputs = self.output_spec.get()
+        #outdir = self.input_spec.outdir
+        #denoising_strategy = input_spec.denoise_type


Let's drop these comments.

effigies · 2017-05-04T17:28:32Z

nipype/interfaces/fsl/ICA_AROMA.py

+        if self.inputs.denoise_type in ('nonaggr', 'both'):
+            outputs['nonaggr_denoised_file'] = os.path.join(out_dir, 'denoised_func_data_nonaggr.nii.gz')
+
+        return outputs


Add a newline at the end of file.

effigies · 2017-05-04T17:30:54Z

nipype/interfaces/fsl/ICA_AROMA.py

+
+class ICA_AROMA(CommandLine):
+    """
+


Should describe what this interface does and link to external documentation. Have a look at some other interfaces for examples. You can largely copy this from any existing ICA_AROMA documentation.

effigies · 2017-05-04T17:31:34Z

nipype/interfaces/fsl/ICA_AROMA.py

+
+class ICA_AROMAInputSpec(CommandLineInputSpec):
+    feat_dir = Directory(exists=True, mandatory=True,
+                        argstr='-feat %s', 


These should be shifted right by one space (aligned with the inside of the parentheses).

chrisgorgo · 2017-05-04T17:36:15Z

nipype/interfaces/fsl/ICA_AROMA.py

+import os
+
+class ICA_AROMAInputSpec(CommandLineInputSpec):
+    feat_dir = Directory(exists=True, mandatory=True,


I don't think feat_dir is mandatory in ICA AROMA.

feat_dir or in_file and friends. xor handles competing mandatory=True traits.

Oh ok - sorry!

chrisgorgo · 2017-05-04T17:37:42Z

nipype/interfaces/fsl/ICA_AROMA.py

+                   desc='path/name of the mat-file describing the '
+                   'affine registration (e.g. FSL FLIRT) of the '
+                   'functional data to structural space (.mat file)')
+    fnirt_warp_file = File(exists=True, mandatory=True,


This also does not seem to be mandatory - ICA AROMA also supports inputs that are in the MNI space.

This may be true. I honestly haven't dug into their documentation to be sure what all is actually mandatory.

Here is their argparse setup: https://github.com/rhr-pruim/ICA-AROMA/blob/master/ICA_AROMA.py#L27-L37

good catch @chrisfilo, fnirt_warp_file and mat_file don't appear to be mandatory

ICA-AROMA checking inputs: https://github.com/rhr-pruim/ICA-AROMA/blob/master/ICA_AROMA.py#L109-L116

I would actually check inside this function, to see what it does: https://github.com/rhr-pruim/ICA-AROMA/blob/master/ICA_AROMA.py#L197

got it: https://github.com/rhr-pruim/ICA-AROMA/blob/master/ICA_AROMA_functions.py#L148-L163

jdkent · 2017-05-04T20:06:17Z

nipype/interfaces/fsl/ICA_AROMA.py

+    >>> AROMA_obj.inputs.mask = 'mask.nii.gz'
+    >>> AROMA_obj.inputs.denoise_type = 'both'
+    >>> AROMA_obj.inputs.out_dir = 'ICA_testout'
+    >>> AROMA_obj.cmdline # doctest: +ALLOW_UNICODE


How do I get rid of the u'string' output? (and I need to to change the motion parameter file)

The u'' part is handled by the +ALLOW_UNICODE flag. The problem here is actually that there's a tab after the newline. You can either remove the spaces before -affmat or just remove the newline entirely and just have the whole string on one line.

effigies · 2017-05-04T20:16:31Z

nipype/interfaces/fsl/ICA_AROMA.py

+    aggr_denoised_file=File(exists=True, 
+                           desc='if generated: aggressively denoised volume')
+    nonaggr_denoised_file=File(exists=True,
+                              desc='if generated: non aggressively denoised volume' )


For both of these traits, add spaces around =, and align your desc with exists.

effigies · 2017-05-04T20:19:06Z

nipype/interfaces/fsl/ICA_AROMA.py

+
+class ICA_AROMA(CommandLine):
+    """
+    Interface for the ICA_AROMA.py script (v0.3 beta).


I'd remove the version number. Unless the interface changes, we don't really need to tag versions, since it'll cause confusion about whether the interface is still valid for future versions. (If it does, then we'll figure out how to make it work with multiple versions.)

effigies · 2017-05-04T20:20:12Z

nipype/interfaces/fsl/ICA_AROMA.py

+    See link for further documentation: https://github.com/rhr-pruim/ICA-AROMA
+
+
+


Limit blank lines (here and above the See link... line) to one.

effigies

Nice. One final question. (Sorry I just noticed this.)

effigies · 2017-05-04T20:46:34Z

nipype/interfaces/fsl/ICA_AROMA.py

+                               '-none: only classification, no denoising '
+                               '-nonaggr (default): non-aggresssive denoising, i.e. partial component regression '
+                               '-aggr: aggressive denoising, i.e. full component regression '
+                               '-both: both aggressive and non-aggressive denoising (two outputs)')


This desc string is:

Type of denoising strategy: -none: only classification, no denoising -nonaggr (default): non-aggresssive denoising, i.e. partial component regression -aggr: aggressive denoising, i.e. full component regression -both: both aggressive and non-aggressive denoising (two outputs)

Is this correct, or do you want it to be:

Type of denoising strategy: -none: only classification, no denoising -nonaggr (default): non-aggresssive denoising, i.e. partial component regression -aggr: aggressive denoising, i.e. full component regression -both: both aggressive and non-aggressive denoising (two outputs)

If the latter, replace the space at the end of each line with \n. If the former is what you intended, no change necessary.

I think I want the latter, thanks! I wasn't sure if newlines were interpreted within single quotes or if I had to go to double quotes (because that makes a difference in bash, I didn't know in python).

In Python, they're the same, you just use whichever looks nicer to you. The only difference is that with double-quotes you have to escape double-quotes and with single-quotes you have to escape single-quotes. (My preference is to use single quotes unless the string contains a single quote, but there is no enforced rule in nipype.)

effigies · 2017-05-04T21:12:34Z

LGTM. Any final comments, @chrisfilo?

jdkent · 2017-05-05T02:03:55Z

I'm afraid I may be in over my head interpreting what I need to change to make this code pass the ci/circleci test. Does this have to do with modifying the docker files?

effigies · 2017-05-05T03:25:14Z

I think it's a random failure. I'll restart the job when I can. Might be in the morning.

…

On May 4, 2017 7:03 PM, "James Kent" ***@***.***> wrote: I'm afraid I may be in over my head interpreting what I need to change to make this code pass the ci/circleci test. Does this have to do with modifying the docker files? — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#1993 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAFF8par0YUdcEmreCbGES2rTTxTms4Cks5r2oOMgaJpZM4NO97u> .

effigies · 2017-05-08T02:23:43Z

@jdkent Can you merge from master? The tests should be fixed.

up to date

effigies · 2017-05-08T20:42:45Z

This looks like it should have passed. @satra did something change with the doctests? Circle is complaining about a missing functional.nii, but nipype/testing/data/functional.nii exists.

effigies · 2017-05-08T20:44:21Z

nipype/interfaces/fsl/ICA_AROMA.py

+    -------
+
+    >>> from nipype.interfaces.fsl import ICA_AROMA 
+    >>> from nipype.testing import example_data


You can remove the example_data import. Shouldn't have any effect on the tests, though.

satra · 2017-05-08T21:26:59Z

nipype/interfaces/fsl/ICA_AROMA.py

@@ -0,0 +1,108 @@
+#ICA_AROMA pulled from: https://github.com/rhr-pruim/ICA-AROMA
+#This assumes ICA_AROMA.py is already installed and callable via $PATH


@effigies, @jdkent - we are missing the standard header that imports the testing paths for doctest. see any other interface file in nipype.

Thanks! Sorry, I'm new to all this.

jdkent · 2017-05-09T15:11:54Z

The last line having trouble in the Travis CI build says this:
nipype/pipeline/engine/tests/test_utils.py::test_mapnode_crash3 /home/travis/.travis/job_stages: line 53: 12967 Segmentation fault (core dumped) py.test -v --doctest-modules nipype

Would this be anything related to how I have ICA_AROMA formatted?

mgxd · 2017-05-09T15:22:49Z

@jdkent that's a very specific and not always reproducible bug we've been encountering in our test builds, unrelated to ICA_AROMA. I've restarted those specific builds

effigies · 2017-05-09T15:30:38Z

@mgxd I've restarted them multiple times. The bug may be getting more reliable...

effigies · 2017-05-10T17:18:14Z

Not going to hold this one up for the segfault bug. Merging.

jdkent added 2 commits May 2, 2017 23:14

Merge pull request #1 from nipy/master

f286f52

keeping up to date

adding python wrapper for ICA AROMA

fafdf22

effigies previously requested changes May 3, 2017

View reviewed changes

Merge pull request #2 from nipy/master

c1beb05

keeping up to date

jdkent added 2 commits May 3, 2017 17:23

WIP: ICA_AROMA.py, adding effigies edits

d4e43b3

WIP: ICA_AROMA.py, add spaces after commas

7ff31d4

WIP: (still learning) changed testing output

4c89009

effigies requested changes May 4, 2017

View reviewed changes

chrisgorgo reviewed May 4, 2017

View reviewed changes

WIP: ICA_AROMA.py, responding to 2nd round of edits

19848fd

jdkent commented May 4, 2017

View reviewed changes

effigies reviewed May 4, 2017

View reviewed changes

WIP: ICA_AROMA.py, fixing my syntactic/formatting mistakes

aeaa109

effigies approved these changes May 4, 2017

View reviewed changes

WIP: ICA_AROMA.py, adding newlines to documentation

aa6db2d

Merge pull request #3 from nipy/master

d4a472d

up to date

effigies reviewed May 8, 2017

View reviewed changes

satra reviewed May 8, 2017

View reviewed changes

added docstring to top

261fade

effigies merged commit e605dfe into nipy:master May 10, 2017

		See link for further documentation: https://github.com/rhr-pruim/ICA-AROMA

		@@ -0,0 +1,108 @@
		#ICA_AROMA pulled from: https://github.com/rhr-pruim/ICA-AROMA
		#This assumes ICA_AROMA.py is already installed and callable via $PATH

WIP: Adding a python wrapper for ICA_AROMA #1993

WIP: Adding a python wrapper for ICA_AROMA #1993

Uh oh!

Conversation

jdkent commented May 3, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

chrisgorgo commented May 3, 2017

Uh oh!

effigies left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

codecov-io commented May 3, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

jdkent commented May 3, 2017

Uh oh!

effigies commented May 3, 2017

Uh oh!

effigies left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

jdkent commented May 3, 2017 •

edited

Loading

codecov-io commented May 3, 2017 •

edited

Loading