[FIX] FSL model.py make multiple F-tests

[AnnaD15]

Hello everybody,

I think there is a bug in the FSL model.py. If one attempts to make one F-contrast the FSL MultipleRegressDesign works fine, however it throws an error when trying to make two F-contrasts saying it cannot append to a string. With the changes below the error should be fixed.

I hope this helps!
Anna

[codecov]

Codecov Report

Merging #3166 into master will increase coverage by 0.29%.
The diff coverage is 0%.

@@            Coverage Diff            @@
##           master   #3166      +/-   ##
=========================================
+ Coverage   67.31%   67.6%   +0.29%     
=========================================
  Files         299     299              
  Lines       39493   39493              
  Branches     5219    5219              
=========================================
+ Hits        26585   26701     +116     
+ Misses      12198   12083     -115     
+ Partials      710     709       -1

Flag	Coverage Δ
#smoketests	53.04% <0%> (ø)	⬆️
#unittests	64.87% <0%> (+0.34%)	⬆️

Impacted Files	Coverage Δ
nipype/pipeline/plugins/sgegraph.py	14.94% <0%> (ø)	⬆️
nipype/utils/docparse.py	52.21% <0%> (ø)	⬆️
nipype/interfaces/fsl/model.py	80.56% <0%> (+0.18%)	⬆️
nipype/interfaces/afni/preprocess.py	82.02% <0%> (+0.22%)	⬆️
nipype/interfaces/freesurfer/preprocess.py	66.04% <0%> (+0.39%)	⬆️
nipype/interfaces/spm/preprocess.py	56.93% <0%> (+0.54%)	⬆️
nipype/interfaces/base/core.py	86.96% <0%> (+0.58%)	⬆️
nipype/interfaces/io.py	59.82% <0%> (+1.16%)	⬆️
nipype/utils/logger.py	89.23% <0%> (+1.53%)	⬆️
nipype/interfaces/afni/base.py	67.22% <0%> (+1.68%)	⬆️
... and 11 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 aa069c0...aa4d604. Read the comment docs.

[effigies]

This looks right to me from the perspective of fixing the Python error, but I have no experience with writing FTS files. Can you confirm it does what you want?

This would also be a good thing to write a regression test for. This would be something like:

# In nipype/interfaces/fsl/tests/test_models.py
# Make sure the following are imported:
from ...pipeline import engine as pe
from ..model MultipleRegressDesign

# Define a test. Here I suggest using the `tmp_path` feature which  just provides
# a `Path()` pointing to an empty directory that will be deleted afterwards.
def test_MultipleRegressDesign_multi_ftest(tmp_path):
    nd = pe.Node(MultipleRegressDesign(), name='nd', basedir=str(tmp_path))
    # Add some inputs that would trigger the error, as small as possible...
    nd.inputs.contrasts = ...
    ...
    # Run the interface; this should fail before your fix and pass after
    res = nd.run()
    # Add some checks on the outputs, for example:
    assert os.path.exists(res.outputs.design_fts)
    design_fts = Path(res.outputs.design_fts)
    assert "<some expected text>" in design_fts.read_text()
    # And any other checks that seem appropriate

[AnnaD15]

@effigies sure I'll try to do this tomorrow!

[AnnaD15]

@effigies

Would this be an appropriate regression test?
The two equivalent F contrast do not make sense regarding the content but it would be a really short test.

#In nipype/interfaces/fsl/tests/test_models.py
from nipype.pipeline import engine as pe
from nipype.interfaces.fsl import MultipleRegressDesign
import os
from pathlib import Path
 
def test_MultipleRegressDesign_multi_ftest(tmp_path):
    nd = pe.Node(MultipleRegressDesign(), name='nd', basedir=str(tmp_path))
    # Add some inputs 
    nd.inputs.contrasts = [('contrast_groups', 'T', ['group1', 'group2'], [1, -1]),
                           ('main_effect', 'F', [('contrast_groups', 'T', ['group1', 'group2'], [1, -1])
                                                ]),
                           ('same_main_effect', 'F', [('contrast_groups', 'T', ['group1', 'group2'], [1, -1])
                                               ]
                           )
                          ]
    nd.inputs.regressors = dict(group1=[1,0], group2=[0,1])
    # Run the interface
    res = nd.run()
    # Checks:
    # Check design_con file
    assert os.path.exists(res.outputs.design_con)
    design_con = Path(res.outputs.design_con)
    assert '/NumContrasts   1' in design_con.read_text()   
    # Check design_fts file
    assert os.path.exists(res.outputs.design_fts)
    design_fts = Path(res.outputs.design_fts)
    assert '1', '1' & '/NumContrasts   2' in design_fts.read_text()

Alternativly more correct regarding the F contrasts would be something like:

#In nipype/interfaces/fsl/tests/test_models.py
from nipype.pipeline import engine as pe
from nipype.interfaces.fsl import MultipleRegressDesign
import os
from pathlib import Path

def test_MultipleRegressDesign_multi_ftest(tmp_path):
    nd = pe.Node(MultipleRegressDesign(), name='nd', basedir=str(tmp_path))
    # Add some inputs 
    nd.inputs.contrasts = [('group_1_mean', 'T', ['group1', 'group2'], [1, 0]),
                           ('group_2_mean', 'T', ['group1', 'group2'], [0, 1]),
                           ('contrast_groups', 'T', ['group1', 'group2'], [1, -1]),
                           ('overall_mean', 'F', [('group_1_mean', 'T', ['group1', 'group2'], [1, 0]),
                                                  ('group_2_mean', 'T', ['group1', 'group2'], [0, 1])
                                                 ]
                           ),
                           ('main_effect', 'F', [('contrast_groups', 'T', ['group1', 'group2'], [1, -1])
                                               ]
                           )
                          ]
    nd.inputs.regressors = dict(group1=[1,0], group2=[0,1])
    # Run the interface
    res = nd.run()
    # Checks:
    # Check design_con file
    assert os.path.exists(res.outputs.design_con)
    design_con = Path(res.outputs.design_con)
    assert '/NumContrasts   3' in design_con.read_text()   
    # Check design_fts file
    assert os.path.exists(res.outputs.design_fts)
    design_fts = Path(res.outputs.design_fts)
    assert '1 1 0', '0 0 1' & '/NumContrasts   2' in design_fts.read_text()

However the regression test fails already before doing the checks in the current nipype version. After the fix, the regression test runs and all checks pass.

[AnnaD15]

I just realized that it is much easier to extend the existing example in nipype/interfaces/fsl/tests/test_models.py, so I changed the code there.

[effigies]

Nice! I have a few suggestions.

[effigies]

We're going to need some way of keeping the outputs in a temporary directory. We can either keep this tmpdir.chdir() or replace foo = ...:

# Above
from ....pipeline import engine as pe  # I think I have enough dots...

# Here
designer = pe.Node(fsl.MultipleRegressDesign(), name='designer', base_dir=str(tmpdir))

This uses the isolation of Node to do the work without having to chdir in the test, which is my personal preference.

(I'm updating foo to designer just because foo is an annoying variable name. You don't need to do that.)

[AnnaD15]

thanks for all the suggestions! I think I changed the code regarding all your suggestions. Is it possible to use the from ....pipeline import engine as pe when testing the code locally too? It did no work for me, instead I had to write from nipype.pipeline import engine as pe .

[effigies]

Relative imports only work within a package. If by "testing the code locally" you mean typing it into a >>> shell, then no, you need to use the full path.

[AnnaD15]

yes, thats what I ment, thanks! Makes sense to me now!

[effigies]

Nice!

[effigies]

I'm not a fan of eval. It's a big heavy hammer for pulling a variable out of the local ether. I would rather do something like:

outputs = res.outputs.get_traitsfree()  # This fetches `outputs` contents as a dict.
for ftype in ["mat", "con", "fts", "grp"]:
    assert Path(outputs["design_" + ftype]).exists()

[effigies]

LGTM.