Skip to content

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

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 8 commits into from
Feb 14, 2020
Merged

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

Show file tree
Hide file tree
Changes from 4 commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
89a66f8
Update model.py
AnnaD15 Feb 10, 2020
6ee5f9e
Update .zenodo.json
AnnaD15 Feb 10, 2020
d338883
Update model.py
AnnaD15 Feb 10, 2020
cb08b18
Update test_model.py
AnnaD15 Feb 13, 2020
cffcbb6
Update nipype/interfaces/fsl/tests/test_model.py
AnnaD15 Feb 13, 2020
830b2bf
Update nipype/interfaces/fsl/tests/test_model.py
AnnaD15 Feb 13, 2020
aa069c0
Update test_model.py
AnnaD15 Feb 13, 2020
aa4d604
Merge branch 'master' into AnnaD15-patch-1
effigies Feb 14, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 5 additions & 0 deletions .zenodo.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -761,6 +761,11 @@
"name": "Mihai, Paul Glad",
"orcid": "0000-0001-5715-6442"
},
{
"affiliation": "Department of Psychology, University of Bielefeld, Bielefeld, Germany.",
"name": "Doll, Anna",
"orcid": "0000-0002-0799-0831"
},
{
"name": "Lai, Jeff"
},
Expand Down
3 changes: 1 addition & 2 deletions nipype/interfaces/fsl/model.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1574,8 +1574,7 @@ def _run_interface(self, runtime):
for tcon in con[2]:
convals[tconmap[self.inputs.contrasts.index(tcon)]] = 1
fcon_txt.append(" ".join(["%d" % val for val in convals]))
fcon_txt = "\n".join(fcon_txt)
fcon_txt += "\n"
fcon_txt = "\n".join(fcon_txt) + "\n"
# write group file
grp_txt = ["/NumWaves 1", "/NumPoints %d" % npoints, "", "/Matrix"]
for i in range(npoints):
Expand Down
13 changes: 6 additions & 7 deletions nipype/interfaces/fsl/tests/test_model.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,23 +6,23 @@
import pytest
import nipype.interfaces.fsl.model as fsl
from nipype.interfaces.fsl import no_fsl
from pathlib import Path


@pytest.mark.skipif(no_fsl(), reason="fsl is not installed")
def test_MultipleRegressDesign(tmpdir):
tmpdir.chdir()
Copy link
Member

@effigies effigies Feb 13, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.)

Copy link
Contributor Author

@AnnaD15 AnnaD15 Feb 13, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 .

Copy link
Member

@effigies effigies Feb 14, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

@AnnaD15 AnnaD15 Feb 17, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

foo = fsl.MultipleRegressDesign()
foo.inputs.regressors = dict(
voice_stenght=[1, 1, 1], age=[0.2, 0.4, 0.5], BMI=[1, -1, 2]
)
con1 = ["voice_and_age", "T", ["age", "voice_stenght"], [0.5, 0.5]]
con2 = ["just_BMI", "T", ["BMI"], [1]]
foo.inputs.contrasts = [con1, con2, ["con3", "F", [con1, con2]]]
foo.inputs.contrasts = [con1, con2, ["con3", "F", [con1, con2]], ["con4", "F", [con2]]]
Copy link
Member

@effigies effigies Feb 13, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice!

res = foo.run()

for ii in ["mat", "con", "fts", "grp"]:
assert (
getattr(res.outputs, "design_" + ii) == tmpdir.join("design." + ii).strpath
os.path.exists(eval('res.outputs.design_'+ii))
)
Copy link
Member

@effigies effigies Feb 13, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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()


design_mat_expected_content = """/NumWaves 3
Expand All @@ -48,10 +48,11 @@ def test_MultipleRegressDesign(tmpdir):
"""

design_fts_expected_content = """/NumWaves 2
/NumContrasts 1
/NumContrasts 2

/Matrix
1 1
0 1
"""

design_grp_expected_content = """/NumWaves 1
Expand All @@ -63,6 +64,4 @@ def test_MultipleRegressDesign(tmpdir):
1
"""
for ii in ["mat", "con", "fts", "grp"]:
assert tmpdir.join("design." + ii).read() == eval(
"design_" + ii + "_expected_content"
)
assert Path(eval('res.outputs.design_'+ii)).read_text() in eval( "design_" + ii + "_expected_content")