WIP logging and validations in CompCor, SignalExtraction, ApplyTopUp #1676

berleant · 2016-10-06T22:02:51Z

No description provided.

codecov-io · 2016-10-06T22:22:47Z

Current coverage is 70.80% (diff: 94.31%)

Merging #1676 into master will increase coverage by 0.04%

@@             master      #1676   diff @@
==========================================
  Files          1030       1030          
  Lines         51764      51822    +58   
  Methods           0          0          
  Messages          0          0          
  Branches       7327       7336     +9   
==========================================
+ Hits          36628      36690    +62   
+ Misses        14045      14039     -6   
- Partials       1091       1093     +2

Powered by Codecov. Last update 55d052b...fb3c550

… compcor

satra · 2016-10-12T01:24:56Z

@shoshber - there seems to be a lot of changes happening here related to unicode drops. perhaps coming from a python 3 environment and make specs. can you do a make specs on the branch in a python 2 environment?

oesteban · 2016-10-12T03:51:41Z

nipype/interfaces/fsl/epi.py

@@ -354,14 +354,14 @@ class ApplyTOPUP(FSLCommand):

    >>> from nipype.interfaces.fsl import ApplyTOPUP
    >>> applytopup = ApplyTOPUP()
-    >>> applytopup.inputs.in_files = ["epi.nii", "epi_rev.nii"]
+    >>> applytopup.inputs.in_files = ["ds003_sub-01_mc.nii.gz", "ds003_sub-01_mc.nii.gz"]


why this? epi.nii and epi_rev.nii are empty files in testing/data

Bc I added the check for dimensionality, empty files were causing the tests to fail. ds003_sub-01_mc.nii.gz is an already-existing file in testing/data that has the appropriate dimensions. It is a little hacky but consistent with the rest of nipype's testing infrastructure.

Oh I see. The _parse_inputs method is probably not the best place to this kind of check. I think it would be better placed the _run_interface method. Something like:

def _run_interface(self, runtime): ... your inputs sanity checks ... return super(ApplyTopup, self).__run_interface(runtime)

This way the doctests will pass with empty files and we don't alter the expected behavior of _parse_inputs.

I'm removing the test for dimensionality based on others' comments

oesteban · 2016-10-12T03:56:48Z

nipype/algorithms/confounds.py

@@ -402,6 +409,11 @@ class TCompCor(CompCor):
    def _run_interface(self, runtime):
        imgseries = nb.load(self.inputs.realigned_file).get_data()

+        if imgseries.ndim != 4:


Why here you are using the numpy array .ndim property, and you have changed the same in your modification of ApplyTopup to use the nibabel header object? Which one is the appropriate option? If both are equivalent, I prefer this (numpy.ndim) rather than your change to ApplyTopup.

The difference is that using numpy requires loading the data into numpy memory, which is expensive. In fact, in ApplyTopUp, it was erroring out. Here, in TCompCor we are loading data into numpy memory anyway, so it is not an additional cost.

Thanks! That's very sound.

oesteban · 2016-10-12T03:59:26Z

nipype/interfaces/ants/resampling.py

-    transforms = InputMultiPath(
-        File(exists=True), argstr='%s', mandatory=True, desc='transform files: will be applied in reverse order. For example, the last specified transform will be applied first')
+                                            )
+    transforms = traits.Either(InputMultiPath(File(exists=True), desc='transform files: will be '


What is this for?

I wanted to be able to pass an identity transform (use case: ds005 in fmriprep). This way, one can pass either a list of transform files OR the keyword 'identity'. It makes the code much cleaner in fmriprep, where I can say "transform the epi files with transformation ___" for ds005 AND ds054, so I don't have to special-case. In some ways it is a workaround for not having conditional workflows in nipype.

I think this is too specific of fmriprep to be included in nipype.

I propose you to create in fmriprep a wrapper of this interface adding this new 'identity' option, instead of including this modification in nipype, that will affect a lot of users. WDYT?

If you don't pass a --transforms option to antsApplyTransforms it uses the identity. Would this be a better option?

@blakedewey I think that is equivalent to what I did.
@oesteban I designed it to be backwards compatible. Did I miss something? I figured it is a change anyone solving a similar problem would appreciate.

btw, in case it wasn't clear, the keyword 'identity' is accepted by applyTransforms in the command line--this isn't just an @shoshber Invention(TM).

oesteban · 2016-10-12T04:00:27Z

nipype/interfaces/fsl/epi.py

@@ -373,6 +373,12 @@ def _parse_inputs(self, skip=None):
        if skip is None:
            skip = []

+        for filename in self.inputs.in_files:
+            ndims = nib.load(filename).header['dim'][0]


see comment above

oesteban · 2016-10-12T04:04:01Z

For some reason, test_auto_CompCor.py got deleted in #1678.

satra · 2016-10-12T12:46:24Z

@oesteban - since there are direct tests for CompCor test_compcorr, make specs deletes the auto test, which i think is fine.

berleant · 2016-10-12T19:12:17Z

run make specs in python2

Regarding checking the dimensions of nifti files, how useful/difficult would it be to make a 4DNiftiFile trait? Are we worried about performance issues if we check dimensionality all the time?

berleant

responses to @oesteban 's questions

berleant · 2016-10-12T19:20:51Z

nipype/algorithms/confounds.py

@@ -402,6 +409,11 @@ class TCompCor(CompCor):
    def _run_interface(self, runtime):
        imgseries = nb.load(self.inputs.realigned_file).get_data()

+        if imgseries.ndim != 4:


The difference is that using numpy requires loading the data into numpy memory, which is expensive. In fact, in ApplyTopUp, it was erroring out. Here, in TCompCor we are loading data into numpy memory anyway, so it is not an additional cost.

berleant · 2016-10-12T19:30:12Z

nipype/interfaces/ants/resampling.py

-    transforms = InputMultiPath(
-        File(exists=True), argstr='%s', mandatory=True, desc='transform files: will be applied in reverse order. For example, the last specified transform will be applied first')
+                                            )
+    transforms = traits.Either(InputMultiPath(File(exists=True), desc='transform files: will be '


I wanted to be able to pass an identity transform (use case: ds005 in fmriprep). This way, one can pass either a list of transform files OR the keyword 'identity'. It makes the code much cleaner in fmriprep, where I can say "transform the epi files with transformation ___" for ds005 AND ds054, so I don't have to special-case. In some ways it is a workaround for not having conditional workflows in nipype.

blakedewey · 2016-10-12T20:03:52Z

Are we sure we want to be doing these kinds of checks on CommandLine-derived interfaces? Maybe I missed the motivation for this but it seems like this kind of input checking should be left to the command that is being called. Does applytopup not give a useful error in this case? We have lots of interfaces here that require their input files to be in very specific forms but they are not checked to my knowledge. I can see doing it in a Pure Python module where no other checking is done. Thoughts?

oesteban · 2016-10-12T20:34:51Z

@blakedewey I think you are right in general, but only for the case of TOPUP and ApplyTopup I would lean towards @shoshber's solutions. The reason is as follows: both interfaces have very specific inputs, with a lot of caveats and particular details (for example, TOPUP would not accept images with an odd number of voxels in x, y or z). The second reason is, as you mentioned, the errors that come out. These two tools have some meaningful crashes, but not always. If you play around the field you input (i.e. not generated with TOPUP) or the encoding file, etc. some errors are more like just a segfault. It is nice to have some extra checking on these (or at least, give more interpretable errors).

berleant · 2016-10-12T20:41:30Z

@blakedewey @oesteban @chrisfilo The reason I felt comfortable checking for 4-dimensionality here is that the description for in_files in the input spec specified 4-D niftis. However, I just looked in more detail at the FSL documentation and it is indeed valid, sometimes, for the inputs to be 3-dimensional. (http://fsl.fmrib.ox.ac.uk/fsl/fslwiki/topup/ApplytopupUsersGuide)

In our case, we inputted a list of 3-dimensional nifti and received, incorrectly, a 3-D nifti file back. There was no error whatsoever, which means some validation somewhere is necessary, though perhaps a more complicated one. Whether it should be a global (i.e. in nipype) validation or not is indeed a good question.

I await others' thoughts with bated breath.

blakedewey · 2016-10-12T23:35:36Z

Yes I have used applytopup on 3D files. Honestly, I don't know anyone that can describe exactly what applytopup and topup will allow, every once in a while I get an error and have to look it up on the FSL mailing list.

That being said, if you put in a 3D file for applytopup you should get a 3D file back (it is only performing the unwarping).

I would be against global validation as the number of variations to cover would be extreme. For example, we aren't even limited to NIfTI files for FSL (although this is unlikely). Other ITK-based commands are even less limited than FSL.

berleant · 2016-10-19T05:21:21Z

I believe that this should be ready to merge, if @oesteban is satisfied with my answer above regarding identity transforms.

satra · 2016-10-21T02:19:36Z

@shoshber - LGTM

@oesteban - another pair of eyes would be helpful.

oesteban · 2016-10-24T03:16:51Z

nipype/algorithms/tests/test_confounds.py

@@ -24,8 +24,14 @@ def test_fd():
                                          out_file=tempdir + '/fd.txt')
    res = fdisplacement.run()

+    with open(res.outputs.out_file) as all_lines:


Please add from io import open in the beginning to ensure python 2-3 compatibility (http://python-future.org/compatible_idioms.html#file-io-with-open)

oesteban · 2016-10-24T03:23:34Z

I think this is quite ready. My unique concern is the modification of the ants.Resample interface. It'd be worth checking @blakedewey's solution first (not passing any transform is understood as the identity transform), before we modify that interface.

Other than that there is an open directly from built-in. Tests are passing, but it doesn't hurt making sure that we enforce compatibility (and the solution is really quick).

Wrapping up: good job @shoshber !

berleant · 2016-10-24T03:39:15Z

My unique concern is the modification of the ants.Resample interface. It'd be worth checking @blakedewey's solution first (not passing any transform is understood as the identity transform), before we modify that interface.

It turns out this isn't necessary for fmriprep, so I can take it out if you like.

Other than that there is an open directly from built-in. Tests are passing, but it doesn't hurt making sure that we enforce compatibility (and the solution is really quick).

Could you tell me more about this?

oesteban · 2016-10-24T04:07:10Z

@shoshber - regarding the open, I had left a comment on that line, with a link to a reference on how to do it. Regarding the ants interface, I think we are better off not touching things that have been doing well this far.

oesteban

Good job!

oesteban · 2016-11-03T20:49:46Z

nipype/algorithms/confounds.py

@@ -585,7 +626,7 @@ def compute_dvars(in_file, in_mask, remove_zerovariance=False):

    if remove_zerovariance:


We took a closer look on this (w. @chrisfilo) and I realized that you were 100% right about this. Sorry about that.

Since this remove_zerovariance input does not make any sense, let's get rid of it (including the zero_remove function that is more readable inline).

Again, my apologies for not looking at this more thoroughly before.

oesteban · 2016-11-03T20:50:33Z

nipype/algorithms/confounds.py

-    newmask[idx] = tv_mask
-    return newmask
+    new_mask = mask.copy()
+    new_mask[data == 0] = 0


I think we can just do this directly on line 627

oesteban · 2016-11-03T20:51:02Z

nipype/algorithms/tests/test_confounds.py

-from nipype.testing import (assert_equal, example_data, skipif, assert_true)
+from io import open
+
+from nipype.testing import (assert_equal, example_data, skipif, assert_true, assert_in)


chrisgorgo · 2016-11-03T21:00:53Z

@shoshber are you planning to work on this more is is it ready to merge? (it still has "WIP" in the title).

debug tcompcor, add log messages and tests

73b9a0e

satra added the in-progress label Oct 6, 2016

berleant changed the title ~~WIP Debugging/logging in compcor, signalextraction,~~ WIP Debugging/logging in compcor, signalextraction Oct 6, 2016

Shoshana Berleant added 8 commits October 6, 2016 22:37

save outputs to cwd

e811b96

explicity check + error out if mask file and func file don't match in…

0c7c649

… compcor

python3, pep8

8313923

use abs paths; +appropriate way to test this

974a42b

make transforms arg in ApplyTransforms opt

741405f

allow ANTS ApplyTransforms to use identity transform

d38562c

traits mandatory=False is incorrect; ignore unicode in doctests

a512faf

test specs auto

6448ee8

berleant changed the title ~~WIP Debugging/logging in compcor, signalextraction~~ WIP Debugging/logging in compcor, signalextraction; allow 'identity' transform in ants.ApplyTransforms Oct 8, 2016

berleant mentioned this pull request Oct 8, 2016

WIP Confounds nipreps/fmriprep#157

Merged

Shoshana Berleant added 4 commits October 8, 2016 22:13

Add more informative error msg

e98bd95

less mysterious error messages

4f27943

better test

b04d4e7

check and error if input to fsl ApplyTopUp is not 4 dimensional

7c7dcd2

berleant changed the title ~~WIP Debugging/logging in compcor, signalextraction; allow 'identity' transform in ants.ApplyTransforms~~ WIP logging and validations in CompCor, SignalExtraction, ApplyTopUp; allow 'identity' transform in ants.ApplyTransforms Oct 11, 2016

don't load the whole thing into memory

8cfa00f

oesteban reviewed Oct 12, 2016

View reviewed changes

make specs

dd8dfb4

berleant commented Oct 12, 2016

View reviewed changes

Shoshana Berleant added 6 commits October 15, 2016 20:43

pull from nipy/nipype master

b3187f3

add headers to outputs of compcor, framewise displacement + test fix

bba591b

Merge http://github.com/nipy/nipype into debugging

8a660dc

revert 4d validation, fix input spec desc

89b9856

chdir back to original dir before deleting tempdir

a42439f

fix up test (e.g. pep8)

b85fd5f

berleant added needs-review and removed in-progress labels Oct 19, 2016

Merge http://github.com/nipy/nipype into debugging

35a0bb3

oesteban reviewed Oct 24, 2016

View reviewed changes

Shoshana Berleant added 7 commits October 24, 2016 05:19

use from io import open

4f80b2a

revert identity transform

c217a23

Merge http://github.com/nipy/nipype into debugging

cd5385e

try longer timeout

9239f7b

specify tab delimiter

d5e1a0b

don't let divide by zero errors pass by

b48395d

don't calculate var/stddev twice

fb3c550

berleant changed the title ~~WIP logging and validations in CompCor, SignalExtraction, ApplyTopUp; allow 'identity' transform in ants.ApplyTransforms~~ WIP logging and validations in CompCor, SignalExtraction, ApplyTopUp Oct 31, 2016

oesteban approved these changes Nov 3, 2016

View reviewed changes

berleant merged commit c49c042 into nipy:master Nov 9, 2016

		@@ -585,7 +626,7 @@ def compute_dvars(in_file, in_mask, remove_zerovariance=False):

		if remove_zerovariance:

WIP logging and validations in CompCor, SignalExtraction, ApplyTopUp #1676

WIP logging and validations in CompCor, SignalExtraction, ApplyTopUp #1676

Uh oh!

Conversation

berleant commented Oct 6, 2016

Uh oh!

codecov-io commented Oct 6, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Current coverage is 70.80% (diff: 94.31%)

Uh oh!

satra commented Oct 12, 2016

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!

berleant Oct 12, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

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!

berleant Oct 18, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

berleant Oct 18, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

oesteban commented Oct 12, 2016

Uh oh!

satra commented Oct 12, 2016

Uh oh!

berleant commented Oct 12, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

berleant left a comment

Choose a reason for hiding this comment

Uh oh!

berleant Oct 12, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

blakedewey commented Oct 12, 2016

Uh oh!

oesteban commented Oct 12, 2016

Uh oh!

berleant commented Oct 12, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

blakedewey commented Oct 12, 2016

Uh oh!

berleant commented Oct 19, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

satra commented Oct 21, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

oesteban commented Oct 24, 2016

Uh oh!

codecov-io commented Oct 6, 2016 •

edited

Loading

berleant Oct 12, 2016 •

edited

Loading

berleant Oct 18, 2016 •

edited

Loading

berleant Oct 18, 2016 •

edited

Loading

berleant commented Oct 12, 2016 •

edited

Loading

berleant Oct 12, 2016 •

edited

Loading

berleant commented Oct 12, 2016 •

edited

Loading

berleant commented Oct 19, 2016 •

edited

Loading

oesteban commented Oct 24, 2016 •

edited

Loading