[ENH]: add list_fwhm option to create_susan_smooth

[jdkent]

helps with nipreps/fmriprep#706

Changes proposed in this pull request

• add list_fwhms option to create_susan_smooth to allow a list of fwhms to be run, instead of one.

[codecov-io]

Codecov Report

Merging #2233 into master will decrease coverage by 0.01%.
The diff coverage is 27.27%.

@@            Coverage Diff            @@
##           master   #2233      +/-   ##
=========================================
- Coverage   72.31%   72.3%   -0.02%     
=========================================
  Files        1182    1182              
  Lines       58947   58954       +7     
  Branches     8490    8494       +4     
=========================================
- Hits        42628   42627       -1     
- Misses      14935   14944       +9     
+ Partials     1384    1383       -1

Flag	Coverage Δ
#smoketests	72.3% <27.27%> (-0.02%)	⬇️
#unittests	69.87% <27.27%> (-0.02%)	⬇️

Impacted Files	Coverage Δ
nipype/workflows/fmri/fsl/preprocess.py	84.97% <27.27%> (-1.94%)	⬇️
nipype/pipeline/plugins/multiproc.py	76.76% <0%> (-1.41%)	⬇️
nipype/pipeline/engine/utils.py	79.55% <0%> (+0.13%)	⬆️
nipype/interfaces/utility/wrappers.py	86.41% <0%> (+1.23%)	⬆️

---

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 29756d3...ed92f7e. Read the comment docs.

[effigies]

Hi James. This looks good, logic-wise, but I think we can simplify it a bit.

[effigies]

You always want to use the same order, or else you're not getting the Cartesian product. So this should be:

multi_fwhms = [fwhm for in_file in in files for fwhm in fwhms]

[effigies]

Drop the if statement, and just use the first part:

multi_inputs = pe.Node(
    util.Function(function=cartesian_product,
                  output_names=['in_file', 'fwhm', 'usans', 'btthresh']),
    name='multi_inputs')  # It's okay if we never hook this up

smooth = pe.MapNode(
    fsl.SUSAN(), iterfield=['in_file', 'brightness_threshold', 'usans'],
    name='smooth')

I'm also dropping the input_names from Function, as it is now redundant and a potential source of error. And I make the output match the expected input names to SUSAN. Finally, I give more descriptive names to your merge_out and median_out. You don't need to make all of these changes if you feel strongly about them, but at least consider them.

[jdkent]

ah, so if I exclude input_names, the Function interface infers the names of the inputs from the definition of the function? I agree with giving more descriptive names for merge_out and median_out. I also changed change the output_names to cart_<name>, to make it clear they are Cartesian lists (and not redefine usans and btthresh)

[effigies]

Yup. Variable names are accessible parts of function objects. (Not true of output names. That's always just a tuple.)

Those changes sound good.

[effigies]

I would drop this entirely. You can just now accept multiple floats, and treat a single float as a list of length 1.

[effigies]

There's already code to do this work for you.

from nipype.utils.filemanip import filename_to_list
in_files = filename_to_list(in_files)

You can't use this for numbers, but to treat one FWHM as a list of length one:

fwhms = [fwhms] if isinstance(fwhms, (int, float)) else fwhms

[effigies]

It looks like you don't actually use multi_mask_files, so I'd drop mask_files and multi_mask_files from this function entirely.

[effigies]

I think this will be more readable with a multi-connect statement:

susan_smooth.connect([
    (inputnode, multi_inputs, [('in_files', 'in_files'),
                               ('fwhm', 'fwhms')]),
    (median, multi_inputs, [(('out_stat', getbtthresh), 'btthresh')]),
    (merge, multi_inputs, [(('out', getusans), 'usans')]),
    (multi_inputs, smooth, [('in_file', 'in_file'),
                            ('fwhm', 'fwhm'),
                            ('btthresh', 'brightness_threshold'),
                            ('usans', 'usans')]),
    ])

[jdkent]

I think so too, but I didn't want to change the way they had it in case that was their coding standard, but with your recommendation I'll change it.

[effigies]

My feeling is this workflow was written to be readable, as an example for writing them. I think having a bit of a mix where we choose the most readable at any given point is within the spirit of the thing.

[effigies]

This doc update is good, though. Let's keep that.

[effigies]

LGTM.

@jdkent Are you testing downstream to make sure that it's doing what you want?

[jdkent]

@effigies I have tested the workflow locally using a single in_file, mask_file with a list of fwhm, and using a list of in_file, mask_file, with a list of fwhm, they gave expected output. The most basic case is tested in the doc string, yes?

off topic: Do you have any resources for learning how to write tests (unit tests, regression tests) for python repositories like nipype and fmriprep?

[effigies]

The doctest doesn't actually get run during testing (see +SKIP). It's just an example.

How about creating a niworkflows branch that pins against this PR, and then your FMRIPREP branch to that?

In case you haven't updated the nipype pin before:

git fetch upstream
git checkout -b pin/nipype_pr2233 upstream/master
sed -i -e 's/nipy\//jdkent\//' .gitmodules
git submodule sync
git submodule foreach git fetch origin
git submodule foreach git checkout origin/enh-create_susan_smooth
git add .gitmodules nipype
git commit -m 'PIN: jdkent/nipype feature branch'
git push -u origin pin/nipype_pr2233

I think you're familiar with pinning fmriprep against a niworkflows branch?

[effigies]

In fmriprep, our version of regression tests are really just running the entire workflow on truncated datasets with every commit. Our unit tests are sorely lacking.

In nipype, if I'm adding a new feature, I try to look at the tests for nearby features (mostly using grep or searching in GitHub), and see what I can do to exercise the new behavior. Then I mostly depend on Satra to decide whether it's reasonable. And if you're fixing a bug, trying to create a test that fails before but passes after is a good strategy.

But I don't have any references that I regularly recommend. Perhaps I should try to evaluate them some time. Might be worth asking on the brainhack Slack to see if people have suggestions.

[effigies]

Oh, sorry. Before any of that git submodule stuff, you may want to do:

git submodule init
git submodule update

[jdkent]

oops, probably should have done that first, I eventually modified your instructions from the ICA-AROMA commit. I believe it followed something like the following. So the important bit was
git submodule init
git submodule update

cd niworkflows
git submodule init
git submodule update
git checkout -b pin/nipype_pr2233 upstream/master
sed -i -e 's/nipy\//jdkent\//' .gitmodules
git -C nipype fetch origin
git -C nipype checkout origin/enh-create_susan_smooth
git checkout -b update_pin
git add nipype .gitmodules
git commit -m 'PIN: jdkent/nipype feature branch'
git push -u origin pin/nipype_pr2233

[satra]

@effigies - please merge this when ready. i got a little lost with the git submodule comments.

[effigies]

Sounds good. (Submodule stuff is for downstream testing in an fmriprep branch.)

[mgxd]

is this all set?

[jdkent]

I haven't found the time to test it rigorously downstream (in FMRIPREP), but this does work as expected in simpler examples.

[effigies]

I'm okay with merging.

…

On Nov 7, 2017 18:45, "James Kent" ***@***.***> wrote: I haven't found the time to test it rigorously downstream (in FMRIPREP), but this does work as expected in simpler examples. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#2233 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAFF8hys5B3alnA4lOQcozG4KibWL2jqks5s0RWqgaJpZM4P7oer> .