Skip to content
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.

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

ENH: Enable orthogonalization in SPM models via SpecifyModel #2870

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

hstojic
Copy link
Contributor

@hstojic hstojic commented Jan 31, 2019

Summary

SPM 12 has an option to switch off the default orthogonolisation of parametric regressors, but it seems that option cannot be set from Nipype - I implemented it here with simple few lines, supplying orth argument as an additional input in the sessioninfo object. Updated the example and function description.

See discussion in issue #2811

Acknowledgment

  • I acknowledge that this contribution will be available under the Apache 2 license.

@codecov-io
Copy link

Codecov Report

Merging #2870 into master will decrease coverage by 0.69%.
The diff coverage is 20%.

Impacted file tree graph

@@            Coverage Diff            @@
##           master    #2870     +/-   ##
=========================================
- Coverage   67.49%   66.79%   -0.7%     
=========================================
  Files         342      342             
  Lines       43562    43567      +5     
  Branches     5422     5425      +3     
=========================================
- Hits        29402    29101    -301     
- Misses      13458    13723    +265     
- Partials      702      743     +41
Flag Coverage Δ
#smoketests 48.64% <20%> (-1.85%) ⬇️
#unittests 64.88% <0%> (-0.04%) ⬇️
Impacted Files Coverage Δ
nipype/algorithms/modelgen.py 59.27% <20%> (-4.09%) ⬇️
nipype/algorithms/rapidart.py 35% <0%> (-29.42%) ⬇️
nipype/interfaces/spm/base.py 68.64% <0%> (-18.49%) ⬇️
nipype/interfaces/fsl/model.py 75.3% <0%> (-5.32%) ⬇️
nipype/interfaces/spm/preprocess.py 52.88% <0%> (-4.15%) ⬇️
nipype/pipeline/plugins/legacymultiproc.py 61.5% <0%> (-3.5%) ⬇️
nipype/utils/misc.py 65.43% <0%> (-1.24%) ⬇️
nipype/pipeline/engine/workflows.py 77.77% <0%> (-1.18%) ⬇️
nipype/interfaces/io.py 54.36% <0%> (-0.71%) ⬇️
... and 4 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 98beb0a...507aafc. Read the comment docs.

@codecov-io
Copy link

codecov-io commented Feb 4, 2019

Codecov Report

Merging #2870 into master will decrease coverage by 1.73%.
The diff coverage is 0%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #2870      +/-   ##
==========================================
- Coverage   67.49%   65.76%   -1.74%     
==========================================
  Files         342      343       +1     
  Lines       43562    44117     +555     
  Branches     5422     5607     +185     
==========================================
- Hits        29402    29013     -389     
- Misses      13458    14208     +750     
- Partials      702      896     +194
Flag Coverage Δ
#smoketests 42.53% <0%> (-7.97%) ⬇️
#unittests 65.2% <0%> (+0.28%) ⬆️
Impacted Files Coverage Δ
nipype/algorithms/modelgen.py 54.56% <0%> (-8.8%) ⬇️
nipype/algorithms/rapidart.py 35% <0%> (-29.42%) ⬇️
nipype/interfaces/fsl/model.py 55.26% <0%> (-25.35%) ⬇️
nipype/interfaces/spm/base.py 68.64% <0%> (-18.49%) ⬇️
nipype/workflows/fmri/fsl/preprocess.py 72.67% <0%> (-13.39%) ⬇️
nipype/interfaces/fsl/preprocess.py 72.7% <0%> (-9.91%) ⬇️
nipype/pipeline/plugins/linear.py 79.06% <0%> (-6.98%) ⬇️
nipype/utils/subprocess.py 87.31% <0%> (-6.72%) ⬇️
nipype/interfaces/fsl/utils.py 63.8% <0%> (-6.53%) ⬇️
... and 25 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 98beb0a...d3ec0aa. Read the comment docs.

Copy link
Member

@effigies effigies left a comment

Choose a reason for hiding this comment

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

This looks reasonable. A few suggestions.

nipype/algorithms/modelgen.py Show resolved Hide resolved
nipype/algorithms/modelgen.py Outdated Show resolved Hide resolved
@effigies effigies changed the title ENH: adding SPM orth argument to the SpecifyModel class ENH: Enable orthogonalization in SPM models via SpecifyModel Feb 14, 2019
@effigies effigies added this to the 1.1.9 milestone Feb 14, 2019
@satra
Copy link
Member

satra commented Feb 14, 2019

just a note that we may have to do something different between SPM12 and prior versions.

in pre SPM12, orth was sequential and order mattered (and interpretation mattered). this was changed in SPM12, where you can orthgonalize things independently to the same variable, but i have not had a chance to play with this. perhaps @gllmflndn can also help us out :)

@hstojic - if you know how this is done, we should incorporate a version specific option.

@effigies
Copy link
Member

@hstojic Just a heads up that we're going to try to release on Monday, so it'd be good to have any PRs ready by Friday if possible. (No big deal if we have to push off another month.)

I'll let this note stand in for #2857 and #2861 as well.

@hstojic
Copy link
Contributor Author

hstojic commented Feb 21, 2019

just a note that we may have to do something different between SPM12 and prior versions.

in pre SPM12, orth was sequential and order mattered (and interpretation mattered). this was changed in SPM12, where you can orthgonalize things independently to the same variable, but i have not had a chance to play with this. perhaps @gllmflndn can also help us out :)

@hstojic - if you know how this is done, we should incorporate a version specific option.

Good catch. I don't know the SPM code well enough to try to create equivalent version for SPM8. For now i would simply condition this on SPM12, sounds good?

@hstojic
Copy link
Contributor Author

hstojic commented Feb 21, 2019

I tried it out with SPM8, it fails in the estimation step but for some other reason related to the output (below). Design step goes fine, SPM8 simply ignores orth option in fmri_spec, so no need for any special treatment.

I updated the code with @effigies suggestions, pushing it shortly.

[Node] Error on "repsup_susan6_ydiff_test_l1_l1pipeline.analysis.estimate" (/media/hstojic/dataneuro/fnclearning_fmri/dProcessed/nipype_work/repsup_susan6_ydiff_test_l1_l1pipeline/analysis/_subject_s010/estimate)
---------------------------------------------------------------------------
RuntimeError                              Traceback (most recent call last)
<ipython-input-5-27342c4bf114> in <module>()
     16     l1pipeline.run(
     17         'MultiProc',
---> 18         plugin_args = {'n_procs': pars['resources']['n_cores']}
     19     )
     20     l1pipeline._get_inputs()

/home/hstojic/.pyenv/nipy/local/lib/python2.7/site-packages/nipype/pipeline/engine/workflows.pyc in run(self, plugin, plugin_args, updatehash)
    593         if str2bool(self.config['execution']['create_report']):
    594             self._write_report_info(self.base_dir, self.name, execgraph)
--> 595         runner.run(execgraph, updatehash=updatehash, config=self.config)
    596         datestr = datetime.utcnow().strftime('%Y%m%dT%H%M%S')
    597         if str2bool(self.config['execution']['write_provenance']):

/home/hstojic/.pyenv/nipy/local/lib/python2.7/site-packages/nipype/pipeline/plugins/base.pyc in run(self, graph, config, updatehash)
    160                         if result['traceback']:
    161                             notrun.append(
--> 162                                 self._clean_queue(jobid, graph, result=result))
    163                         else:
    164                             self._task_finished_cb(jobid)

/home/hstojic/.pyenv/nipy/local/lib/python2.7/site-packages/nipype/pipeline/plugins/base.pyc in _clean_queue(self, jobid, graph, result)
    222 
    223         if str2bool(self._config['execution']['stop_on_first_crash']):
--> 224             raise RuntimeError("".join(result['traceback']))
    225         crashfile = self._report_crash(self.procs[jobid], result=result)
    226         if jobid in self.mapnodesubids:

RuntimeError: Traceback (most recent call last):
  File "/home/hstojic/.pyenv/nipy/local/lib/python2.7/site-packages/nipype/pipeline/plugins/multiproc.py", line 69, in run_node
    result['result'] = node.run(updatehash=updatehash)
  File "/home/hstojic/.pyenv/nipy/local/lib/python2.7/site-packages/nipype/pipeline/engine/nodes.py", line 471, in run
    result = self._run_interface(execute=True)
  File "/home/hstojic/.pyenv/nipy/local/lib/python2.7/site-packages/nipype/pipeline/engine/nodes.py", line 555, in _run_interface
    return self._run_command(execute)
  File "/home/hstojic/.pyenv/nipy/local/lib/python2.7/site-packages/nipype/pipeline/engine/nodes.py", line 635, in _run_command
    result = self._interface.run(cwd=outdir)
  File "/home/hstojic/.pyenv/nipy/local/lib/python2.7/site-packages/nipype/interfaces/base/core.py", line 523, in run
    outputs = self.aggregate_outputs(runtime)
  File "/home/hstojic/.pyenv/nipy/local/lib/python2.7/site-packages/nipype/interfaces/base/core.py", line 597, in aggregate_outputs
    predicted_outputs = self._list_outputs()
  File "/home/hstojic/.pyenv/nipy/local/lib/python2.7/site-packages/nipype/interfaces/spm/model.py", line 283, in _list_outputs
    outtype = 'nii' if '12' in self.version.split('.')[0] else 'img'
AttributeError: 'NoneType' object has no attribute 'split'

@satra
Copy link
Member

satra commented Feb 21, 2019

@hstojic - that particular error is because for some reason it cannot determine spm version: self.version

not. Same as in SPM, use 'Yes' to indicate orthogonalisation, and 'No'
to explicitly prevent it. Use None for conditions where it is not being
used. Note that by default SPM will orthogonalise parametric regressors
in the order in which they are entered.
Copy link
Member

Choose a reason for hiding this comment

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

can we enable the option where order does not matter? this was the fix in spm12. it's not actually fine to create orthogonalization options where order is important.

Copy link
Member

Choose a reason for hiding this comment

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

it's possible this is the default, in which case the sentence above should be fixed.

Copy link
Member

Choose a reason for hiding this comment

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

default == independent orthogonalization

Copy link
Contributor Author

Choose a reason for hiding this comment

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

As far as I know in SPM12 order still matters bcs orthogonalisation is done with respect to regressors entered previously. SPM8 was doing orthogonalisation automatically and switching it off required a hacky solution, while SPM12 does it by default, but now you can instruct it not to do it. I know this type of orthogonalisation is not necessarily OK, here I am simply implementing this option to switch it off.

Copy link
Member

Choose a reason for hiding this comment

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

Copy link
Member

Choose a reason for hiding this comment

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

@gllmflndn - is there a way to enable this programmatically?

@effigies effigies mentioned this pull request Feb 22, 2019
12 tasks
@effigies effigies modified the milestones: 1.1.9, 1.2.0 Feb 24, 2019
@mgxd
Copy link
Member

mgxd commented Mar 27, 2019

hi @hstojic - is this ready for another review? anything I can do to help?

@hstojic
Copy link
Contributor Author

hstojic commented Mar 28, 2019

To my understanding, @satra is trying to inquire with @gllmflndn about enabling it for SPM8.

Otherwise, I would be fine with the solution as is, we can simply state it will work only with SPM12 where that option exists.

@effigies effigies modified the milestones: 1.2.0, 1.2.1 May 8, 2019
@effigies effigies modified the milestones: 1.2.1, 1.2.2 Aug 16, 2019
@effigies
Copy link
Member

effigies commented Sep 5, 2019

@satra @gllmflndn Just a bump on this one. Any news?

@oesteban oesteban modified the milestones: 1.2.2, 1.3.0, 1.2.3 Sep 5, 2019
@effigies effigies modified the milestones: 1.2.3, future Sep 18, 2019
@carolfs
Copy link

carolfs commented Feb 16, 2021

Is there any news on this pull request? I've been applying this patch to my nipype installation because this option is very useful.

@YukunQu
Copy link

YukunQu commented Jan 25, 2022

I didn't find the change in last version nipype 1.7.0. Is it discarded or some bugs or another way to excute?

@carolfs
Copy link

carolfs commented Jul 7, 2022

This option is so useful. Hoping it will be added to nipype soon.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

8 participants