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

Add option to alternative MNI templates for output spaces #11

Closed
feilong opened this issue Apr 26, 2017 · 24 comments

Comments

Projects
6 participants
@feilong
Copy link

commented Apr 26, 2017

From discussions of another issue (poldracklab/fmriprep#337 (comment)), it would be nice to have alternatives to MNI152NLin2009cAsym as output spaces to increase backward compatibility with other softwares, datasets, and atlases. For example, currently widely used MNI152NLin6Asym.

I'll try to modify fmriprep/workflows/anatomical.py and add output_spaces as an input parameter to init_anat_preproc_wf, and change the spatial normalization code accordingly. Does that sound good? @chrisfilo

@effigies

This comment has been minimized.

Copy link
Collaborator

commented Apr 26, 2017

If I can make a suggestion, what might be nice would be to add a --template-space={MNI152NLin2009cAsym,MNI152NLin6Asym,<etc>} and change the MNI option in --output-space to template. This keeps --output-space options to a reasonably short list of manageable strings.

Unless we think people are potentially going to want their data in many template spaces?

@chrisgorgo

This comment has been minimized.

Copy link
Collaborator

commented Apr 26, 2017

@feilong this change would require modifications of the RobustMNINormalization Interface in niworkflows: https://github.com/poldracklab/niworkflows/blob/master/niworkflows/anat/mni.py#L35. The target atlas would also have to be included in the data downloader https://github.com/poldracklab/niworkflows/blob/master/niworkflows/data/getters.py#L19 (Although @oesteban might know if it is already included - if not if you can email it to me I will add it to our OSF repository).

@effigies Your proposal does reflect the fact that only one volumetric template would be allowed (which should be fine for 99% of users). I would however insist on keeping the full names of the template used in output file names (instead of "template") for provenance related reasons..

@effigies

This comment has been minimized.

Copy link
Collaborator

commented Apr 26, 2017

Yeah. Keeping the full name in filenames makes sense. Just from a UI perspective, not having to write out the full template unless changing from the default would be nice.

@effigies

This comment has been minimized.

Copy link
Collaborator

commented Apr 26, 2017

Actually, speaking of this, should creating structurals in MNI space depend on --output-space? Currently t1_2_mni and the mni_* nodes run no matter what.

@feilong

This comment has been minimized.

Copy link
Author

commented Apr 26, 2017

@effigies I think so, and I guess it might be good to add template space names or abbreviations to those node names?

@oesteban

This comment has been minimized.

Copy link
Contributor

commented Apr 26, 2017

@feilong do you refer to this atlas -> http://nist.mni.mcgill.ca/?p=858 ? It is not currently included in niworkflows. As @chrisfilo said, you'd need to add it.

@chrisgorgo

This comment has been minimized.

Copy link
Collaborator

commented Apr 26, 2017

MNI152NLin6Asym is an asymmetrical version of this atlas - we would have to grab it from SPM or FSL.

@chrisgorgo

This comment has been minimized.

Copy link
Collaborator

commented May 10, 2017

@feilong I'm trying to prioritize features - would you use/benefit from addition of MNI152NLin6Asym?

@feilong

This comment has been minimized.

Copy link
Author

commented May 10, 2017

@chrisfilo Yes, I want my data resampled to both MNI152NLin6Asym and MNI152NLin2009cAsym if possible.

@effigies

This comment has been minimized.

Copy link
Collaborator

commented May 10, 2017

@chrisfilo is it worth updating --template to take multiple template specifiers?

@chrisgorgo

This comment has been minimized.

Copy link
Collaborator

commented May 10, 2017

Interesting. So you would like outouts in both MNI152NLin6Asym and MNI152NLin2009cAsym? Despite it would take double the space?

We could either iterate over T1w coregistration targets or always use MNI152NLin2009cAsym and in the final stage apply one more non-linear warp from MNI152NLin2009cAsym to MNI152NLin6Asym. Such warp should not be hard to estimate.

@feilong

This comment has been minimized.

Copy link
Author

commented May 10, 2017

I need some compatibility at this moment, but eventually MNI152NLin2009cAsym would be the choice.
I guess coregister to the target template directly would be more more accurate, though it would take a little longer.

@dangom

This comment has been minimized.

Copy link

commented Mar 10, 2018

Hi guys, would you have any updates on this issue? I was checking out fmriprep and was a bit frustrated that the template is fixed. For resting-state studies, I'd like to use a dual regression approach against maps generated in MNI 6th gen 2mm. I don't know the consequences of doing this if my maps are in MNI 2009.

Besides that, ICA AROMA hyperplane parameters were developed for MNI 6th 2mm, as seen in the code here and here and in the Manual. Does this mean that, when the --usa-aroma flag is on, registration to MNI 6th is actually performed but then discarded? I reckon that the technique may also work in other templates, but it'd be nice to document that.

Finally, would there be a way of quickly hacking fmriprep into using MNI 6th 2mm by overwriting the MNI 2009 file? Any ideas if things would break?

(by the way, thanks a lot for this project)

@chrisgorgo

This comment has been minimized.

Copy link
Collaborator

commented Mar 10, 2018

Hi guys, would you have any updates on this issue? I was checking out fmriprep and was a bit frustrated that the template is fixed. For resting-state studies, I'd like to use a dual regression approach against maps generated in MNI 6th gen 2mm. I don't know the consequences of doing this if my maps are in MNI 2009.

Considering usual level of smoothness I would not expect a large difference, but we have not tested this explicitly. In principle we have nothing against adding an option to support other target templates (as you probably noticed the command line flag have been designed to accommodate that feature in the future). This could be something that you can help us to add by submitting a pull request.

Besides that, ICA AROMA hyperplane parameters were developed for MNI 6th 2mm, as seen in the code here and here and in the Manual. Does this mean that, when the --usa-aroma flag is on, registration to MNI 6th is actually performed but then discarded? I reckon that the technique may also work in other templates, but it'd be nice to document that.

No - as indicated by the file naming convention of the outputs, transformations to 2009c template are used in the AROMA pipeline. I would be surprised if using MNI 6th would make a significant difference in this context.

Finally, would there be a way of quickly hacking fmriprep into using MNI 6th 2mm by overwriting the MNI 2009 file? Any ideas if things would break?

It would break my heart :) We would much more appreciate if you could work towards implementing this as part of the core package extending the --template flag. We can provide guidance in the process.

(by the way, thanks a lot for this project)
Thank you for the kind words and the feedback. You might also read this thread on neurostars

@jdkent

This comment has been minimized.

Copy link

commented Mar 10, 2018

It would break my heart :)

😆

@dangom

This comment has been minimized.

Copy link

commented Mar 12, 2018

Thanks for the quick and through answer. =)
What would be the necessary changes to get this to work?

this change would require modifications of the RobustMNINormalization Interface in niworkflows: https://github.com/poldracklab/niworkflows/blob/master/niworkflows/anat/mni.py#L35. The target atlas would also have to be included in the data downloader https://github.com/poldracklab/niworkflows/blob/master/niworkflows/data/getters.py#L19

Does the above still hold true?
I won't have time to tackle an implementation within the next few weeks, though.

@oesteban oesteban transferred this issue from poldracklab/fmriprep Jan 8, 2019

@oesteban oesteban added the templates label Jan 8, 2019

@oesteban

This comment has been minimized.

Copy link
Contributor

commented Jan 18, 2019

With #45 we are pretty close to getting this done. It would be really helpful if @effigies brought poldracklab/fmriprep#1115 from fmriprep. I can take the token from there. [That PR would also be helpful with lesion related issues]

@oesteban oesteban added this to To do in 0.0.5 via automation Jan 18, 2019

@oesteban oesteban moved this from To do to In progress in 0.0.5 Jan 18, 2019

@effigies

This comment has been minimized.

Copy link
Collaborator

commented Jan 18, 2019

@oesteban To be clear, that PR never worked on fMRIPrep, and I never had time to figure out why it didn't. If you think it will be helpful, I can try to get it to a similarly broken state against sMRIPrep master, but it's not obvious to me that it's worth the time.

@oesteban

This comment has been minimized.

Copy link
Contributor

commented Jan 18, 2019

I see. Basically, I didn't want to throw your commits away. So, if you think that trying to bring that PR back will have more friction than just start over then I'm happy to take care of the issue.

@effigies

This comment has been minimized.

Copy link
Collaborator

commented Jan 18, 2019

I can see if it's fast. I'll try merging master on fMRIPrep, and if so, then I can just apply the diff in one shot over here.

@effigies

This comment has been minimized.

Copy link
Collaborator

commented Jan 18, 2019

I don't think this is plausibly addressable until we settle on niworkflows >= 0.6. There are currently too many moving parts with regard to these workflows, and trying to wedge this into pre-template-flow (or partial-template-flow? I don't actually know what the 0.5.x state is) code while also trying to move with post-template-flow is just asking for merge conflicts.

Now that we have 0.6, should we just get fMRIPrep/sMRIPrep to support it?

oesteban added a commit to oesteban/niworkflows that referenced this issue Jan 18, 2019

[FIX] Allow arbitrary template names in ``RobustMNINormalization``
The current Enum is precluding the implementation of
poldracklab/smriprep#11
@oesteban

This comment has been minimized.

Copy link
Contributor

commented Jan 18, 2019

I don't see any inconvenient other than the PR to niworkflows just referenced above to allow changing the --template target for smriprep (and fmriprep in the future). I'm just testing it out and I'm positive that it will work out.

A different issue is allowing several templates to be used as standard spaces. That will definitely require the implementation of something like poldracklab/fmriprep#1115.

@effigies

This comment has been minimized.

Copy link
Collaborator

commented Jan 18, 2019

Right, and poldracklab/fmriprep#1115 depends on poldracklab/niworkflows#236, which I've just rebased onto master, having to deal with template-flow changes in poldracklab/niworkflows#283.

It's not clear to me that rebasing poldracklab/niworkflows#283 onto maint/0.5 in order to get poldracklab/fmriprep#1115 working with niworkflows 0.5.x is going to be a good use of time, when we're going to have to turn around and make everything compatible with niworkflows 0.6.

@oesteban

This comment has been minimized.

Copy link
Contributor

commented Jan 18, 2019

It's not clear to me that rebasing poldracklab/niworkflows#283 onto maint/0.5 in order to get poldracklab/fmriprep#1115 working with niworkflows 0.5.x is going to be a good use of time

Totally agree with this. I would work from master (i.e. niworkflows-0.6.x)

oesteban added a commit to oesteban/smriprep that referenced this issue Jan 18, 2019

[ENH] Allow templates other than ``MNI152NLin2009cAsym``
This is possible leveraging TemplateFlow. This PR closes poldracklab#11, although
it does not implement the feature of allowing setting more than just one
spatial normalization target.

Modified ds054's build in CircleCI to test against ``MNI152Lin``.

@oesteban oesteban closed this in #47 Jan 24, 2019

0.0.5 automation moved this from In progress to Done Jan 24, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.