ENH: fsLR output space, cifti output in HCP grayordinate space #1887
Conversation
|
Thank your for raising your pull request. Some of the fMRIPRep maintainers will review your changes as soon as time permits. ## PR Review
*Please check off boxes as applicable, and elaborate in comments below. Your review is not limited to these topics, as described in the reviewer guide*
- [ ] As the reviewer I confirm that there are no conflicts of interest for me to review this work.
Please check what applies in the following aspects of the PR:
#### Code documentation
- [ ] New functions and modules are documented following the guidelines.
- [ ] Existing code required amendments in documentation and has been updated.
- [ ] Sufficient inline comments ensure readability by other contributors in the long term.
#### Documentation site
- [ ] The modifications to *fMRIPrep* in this PR **do not require extra documentation**. This is a common situation when a bug fix that does not change the code base substantially is submitted.
- [ ] This PR requires documentation. The corresponding documentation will be submitted as an additional PR in the future.
- [ ] This PR requires extra documentation, and will be included within this PR. Please check the boxes that apply best:
- [ ] An existing feature is substantially modified, changing the interface and/or logic of a module.
- [ ] A new feature is being added, therefore full documentation of it will be included
- [ ] This PR addresses an error in existing documentation.
- [ ] Yes, all expected sections of documentation were generated by the CI build, and look as expected
#### Tests
- [ ] The new functionalities in this PR are covered by the existing tests
- [ ] New test batteries have been included with this PR
#### Data
- [ ] This PR does not require any additional data to be uploaded to OSF.
- [ ] This PR requires additional data for tests.
#### Dependencies: smriprep
- [ ] This PR does not require any update on `smriprep`; or
- [ ] `smriprep` has correctly been pinned.
#### Dependencies: niworkflows
- [ ] This PR does not require any update on `niworkflows`; or
- [ ] `niworkflows` has correctly been pinned.
#### Dependencies: sdcflows
- [ ] This PR does not require any update on `sdcflows`; or
- [ ] `sdcflows` has correctly been pinned.
#### Dependencies: Nipype
- [ ] This PR does not require any update on `nipype`; or
- [ ] `nipype` has correctly been pinned.
#### Dependencies: other
- [ ] This PR does not require any update on other dependencies; or
- [ ] other dependencies have correctly been pinned.
#### Reports generated within CI tests
- [ ] Yes, I have checked the reports of ds005 and they are not modified or the changes are as expected
- [ ] Yes, I have checked the reports of ds054 and they are not modified or the changes are as expected
- [ ] Yes, I have checked the reports of ds010 and they are not modified or the changes are as expected
### Review Comments (other than those left inline with the code)
|
|
One concern I wanted to raise: because we are resampling the BOLD timeseries to high-res A few ideas of how to address this (would love to hear what others think):
|
See #1892, this is exactly the kind of question that issue is raising. I think this is the way to go (not dumping anything to the output folder unless it is explicitly listed in |
.circleci/ds005_outputs.txt
Outdated
| fmriprep/sub-01/func/sub-01_task-mixedgamblestask_run-01_desc-confounds_regressors.json | ||
| fmriprep/sub-01/func/sub-01_task-mixedgamblestask_run-01_desc-confounds_regressors.tsv | ||
| fmriprep/sub-01/func/sub-01_task-mixedgamblestask_run-01_desc-MELODIC_mixing.tsv | ||
| fmriprep/sub-01/func/sub-01_task-mixedgamblestask_run-01_space-fMRIPrep_bold.dtseries.json |
There was a problem hiding this comment.
shouldn't this be
| fmriprep/sub-01/func/sub-01_task-mixedgamblestask_run-01_space-fMRIPrep_bold.dtseries.json | |
| fmriprep/sub-01/func/sub-01_task-mixedgamblestask_run-01_space-fsaverage_den-10k_bold.dtseries.json |
?
There was a problem hiding this comment.
Related question: how much time would add to run this with --output-spaces fsLR <...> --cifti-outputs?
There was a problem hiding this comment.
does space-fsaverage imply subcortical structures in MNI152NLin2009cAsym?
There was a problem hiding this comment.
not sure exactly how long, the biggest addon would be resampling to fsaverage:164k (resampling to fsLR is fairly quick). Would also spike the size of outputs, as BOLD in fsaverage is fairly big.
If this sounds reasonable, I can add it in
fmriprep/workflows/bold/base.py
Outdated
| @@ -923,10 +924,12 @@ def init_func_preproc_wf( | |||
| # SURFACES ################################################################################## | |||
| surface_spaces = [space for space in output_spaces.keys() if space.startswith('fs')] | |||
| if freesurfer and surface_spaces: | |||
| fslr_density = output_spaces.get('fsLR', {}).get('den', '32k') | |||
There was a problem hiding this comment.
We'll need to address #1895 if we want this to work.
There was a problem hiding this comment.
i've opened nipreps/smriprep#145 - since that isn't exclusive to this I'll submit a fix separate from this. A quick workaround is to specify at least one parameter for fsLR/fsaverage
There was a problem hiding this comment.
Please update the docs at the required points, e.g.:
- https://github.com/poldracklab/fmriprep/blob/master/docs/outputs.rst#preprocessed-data-fmriprep-derivatives
- Add a subsection for HCP Grayordinates after https://github.com/poldracklab/fmriprep/blob/master/docs/workflows.rst#epi-sampled-to-freesurfer-surfaces
- https://github.com/poldracklab/fmriprep/blob/master/docs/spaces.rst#standard-spaces
|
I guess nipreps/niworkflows#436 and #1906 are the blockers now - correct? |
|
yes, we'll need to get nipreps/niworkflows#436 in and cut a new release before merging this. |
| automatically added to the list of output spaces (option ``--output-space``).""", file=stderr) | ||
| if opts.cifti_output: | ||
| grayords = {'91k': '32k', '170k': '59k'} # CIFTI total grayords to surface densities | ||
| output_spaces['fsLR'] = {'den': grayords[opts.cifti_output]} |
There was a problem hiding this comment.
Do you really want to force GIFTI outputs if you're having CIFTI outputs? If they're not explicitly asking for it, it's a lot of duplicated output.
There was a problem hiding this comment.
that's how we are currently handling CIFTIs, i.e. fsaverage5/6 surfaces are always produced atm.
I hope to get this functionality in, and then address the inherent problem after (#1892)
| if 'fsLR' in output_spaces: | ||
| output_spaces['fsLR'] = {'den': output_spaces.get('fsLR', {}).get('den') or '32k'} | ||
| # resample to fsLR from highest density fsaverage | ||
| output_spaces['fsaverage'] = {'den': '164k'} |
There was a problem hiding this comment.
Similarly, now asking for CIFTIs in fsLR means that I'm also getting GIFTI outputs in fsaverage space.
| @@ -940,26 +946,37 @@ def init_func_preproc_wf( | |||
| *Grayordinates* files [@hcppipelines], which combine surface-sampled | |||
| data and volume-sampled data, were also generated. | |||
| """ | |||
| cifti_volume = "MNI152NLin6Asym" if 'fsLR' in cifti_spaces else "MNI152NLin2009cAsym" | |||
There was a problem hiding this comment.
I thought they used an FSL-packaged (and slightly weird) version of the template. Do we need to account for that?
There was a problem hiding this comment.
we are using the same labelled MNI template the HCP folks use to generate the CIFTIs, which should be in same space as the template - what do you mean by slightly weird?
There was a problem hiding this comment.
I might be misremembering. During the TemplateFlow changeover we switched from using FSL's packaged template (MNI152NLin2009cAsym) to MNI's, and there were some differences. I thought using FSL's distribution was something we'd copied from HCP. @oesteban will probably know better.
There was a problem hiding this comment.
I thought they used an FSL-packaged (and slightly weird) version of the template. Do we need to account for that?
The "MNI" distributed with FSL is an unofficial version created by A. Janke with the same data and methodology of the official MNI152NLin6Sym atlas. We called this variant MNI152NLin6Asym in TemplateFlow, although it is not an official MNI template.
Then, the HCP folks resampled the 1mm^3 grid to different resolutions that seem useful to them. We originally placed these files under the fsLR ID in TemplateFlow. @mgxd refactored TemplateFlow and moved them to their (more proper) place in MNI152NLin6Asym.
During the TemplateFlow changeover, we did not switch from the official, latest MNI template (MNI152NLin2009cAsym) to MNI's (I guess you were referring to MNI152NLin6Asym). What really happened is that some submodules (and particularly, ICA-AROMA) stopped using the MNI152NLin2009cAsym template. After the changeover, if you set --ica-aroma then the MNI152NLin6Asym is added to the output-spaces list because ICA-AROMA was originally trained on that space.
Hopefully, this clarifies the spotty connections between TemplateFlow, the "MNI" template(s) and this PR.
|
Apologies if my comments recapitulate earlier arguments. I haven't been following this PR closely. |
|
@oesteban any other concerns? |
Closes #1537, #1673
Changes proposed in this pull request
fsLRsupport to--output-spaces, which involves a two step resample (first tofsaverage:den-164, then tofsLR:den-<den>) [Incomplete implementation of fsLR output space #1673]--cifti-outputis now compliant with HCP grayordinate space (cortex infsLR, subcortical inMNI152NLin6Asym)fsLRis specified inoutput-spacesfsaverage5/6,MNI152NLin2009cAsym) has been removedspacetag to the cifti output, with the new default asspace-fsLRand the old behavior asspace-fMRIPrep--cifti-output [grayordinates]fsLRsurface density (91k corresponds to 32k surfaces,170kto 59kBlocking
niworkflowsdependency to include the above PRtemplateflowto0.4.2rc1