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 assessment of spatial coverage within both fMRIVolume and fMRISurface #138

Merged

Conversation

mharms
Copy link
Contributor

@mharms mharms commented Oct 1, 2019

This PR is intended to address #132. This functionality is a necessary addition before we start HCP-D/A processing.

Some minor readability improvements and comment additions are included for the affected scripts as well.

@mharms mharms requested review from coalsont and glasserm October 1, 2019 20:08
echo "PctCoverage, Nnonzero, Ngrayordinates" >| "$OutputAtlasDenseTimeseries"_nonzero.stats.txt
echo "${PctCoverage}, ${Nnonzero}, ${Ngrayordinates}" >> "$OutputAtlasDenseTimeseries"_nonzero.stats.txt
# If we don't have full grayordinate coverage, save out a mask to identify those locations
if [ "$Nnonzero" -ne "$Ngrayordinates" ]; then
Copy link
Member

Choose a reason for hiding this comment

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

I'm not particularly fond of having an internal aspect of the data determine whether an output file gets generated. If we added some automated fix for the zeros (more dilation), having the mask of zeros exist regardless of whether it is empty might make the code simpler.

Copy link
Contributor Author

@mharms mharms Oct 3, 2019

Choose a reason for hiding this comment

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

I did that here because due to the dilation, zeros in the CIFTI should be rare and the *_nonzero.stats.txt is written out regardless, so that provides the "evidence" that the check was performed on a particular set of processed data. That said, if you and @glasserm prefer that we write out the *_nonzero.dscalar.nii file regardless, I can do that.

I don't see how adding more dilation impacts whether or not we should write out the map of nonzero grayordinates. At some point, we can't just keep dilating, and frankly it seems like we already have a lot of dilation built in -- if I'm reading RibbonVolumeToSurfaceMapping correctly, we already dilate the "native" mesh data by 10 mm, and then dilate by 30 mm after we resample to the 32k_fsLR mesh.

Ideally, it would be great to have a dscalar that captures the grayordinates that contain "dilated" values themselves, but I don't know how easy that would be to implement...

Copy link
Contributor

Choose a reason for hiding this comment

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

I prefer Mike's solution

@@ -23,8 +23,8 @@ Usage() {
echo " --motionmatdir=<input motion correcton matrix directory>"
echo " --motionmatprefix=<input motion correcton matrix filename prefix>"
echo " --ofmri=<input fMRI 4D image>"
echo " --freesurferbrainmask=<input FreeSurfer brain mask, nifti format in T1w space>"
echo " --biasfield=<input biasfield image, in T1w space>"
echo " --freesurferbrainmask=<input FreeSurfer brain mask, nifti format in atlas (MNI152) space>"
Copy link
Member

Choose a reason for hiding this comment

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

I don't know if this is correct.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Here are the inputs to OneStepResampling, from the call in GenericfMRIVolumeProcessingPipeline:

${RUN} ${PipelineScripts}/OneStepResampling.sh \
       --workingdir=${fMRIFolder}/OneStepResampling \
       --infmri=${fMRIFolder}/${OrigTCSName}.nii.gz \
       --t1=${AtlasSpaceFolder}/${T1wAtlasName} \
       --fmriresout=${FinalfMRIResolution} \
       --fmrifolder=${fMRIFolder} \
       --fmri2structin=${T1wFolder}/xfms/${fMRI2strOutputTransform} \
       --struct2std=${AtlasSpaceFolder}/xfms/${AtlasTransform} \
       --owarp=${AtlasSpaceFolder}/xfms/${OutputfMRI2StandardTransform} \
       --oiwarp=${AtlasSpaceFolder}/xfms/${Standard2OutputfMRITransform} \
       --motionmatdir=${fMRIFolder}/${MotionMatrixFolder} \
       --motionmatprefix=${MotionMatrixPrefix} \
       --ofmri=${fMRIFolder}/${NameOffMRI}_nonlin \
       --freesurferbrainmask=${AtlasSpaceFolder}/${FreeSurferBrainMask} \
       --biasfield=${AtlasSpaceFolder}/${BiasFieldMNI} \
       --gdfield=${fMRIFolder}/${NameOffMRI}_gdc_warp \
       --scoutin=${fMRIFolder}/${OrigScoutName} \
       --scoutgdcin=${fMRIFolder}/${ScoutName}_gdc \
       --oscout=${fMRIFolder}/${NameOffMRI}_SBRef_nonlin \
       --ojacobian=${fMRIFolder}/${JacobianOut}_MNI.${FinalfMRIResolution}

--freesurferbrainmask, --biasfield (and --t1) are all from ${AtlasSpaceFolder} ("MNINonLinear"). If it is somehow incorrect to refer to those inputs as "atlas (MNI152) space" rather than "T1w space" in the usage statement, I'll change it back, but if that's that case, would like to hear from @glasserm why my change is incorrect.

Copy link
Member

Choose a reason for hiding this comment

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

I was not claiming it was wrong, just that I didn't know, and highlighting it so someone who did know would double-check.

Copy link
Contributor

Choose a reason for hiding this comment

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

Mike seems correct

Copy link
Contributor

@glasserm glasserm left a comment

Choose a reason for hiding this comment

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

I approve these changes provided they can be merged with Grega's pull requests. Mike please investigate this prior to merge.

@mharms
Copy link
Contributor Author

mharms commented Oct 9, 2019

@coalsont: In light of @glasserm's comments on this PR, do you give it your approval for merging?

@glasserm
Copy link
Contributor

glasserm commented Oct 9, 2019

Mike, can this be merged with the other PRs?

@mharms
Copy link
Contributor Author

mharms commented Oct 9, 2019

It will have a merge conflict with #141, but that PR is a work-in-progress, and sounds like it might change considerably. My changes are relatively minor overall, so it shouldn't be hard for @sivcek to pull-in the new master, and work from there. In fact, I think it would be good if we get this merged in before he does anything else with that PR.

@glasserm glasserm merged commit 4850bbe into Washington-University:master Oct 9, 2019
@sivcek
Copy link
Contributor

sivcek commented Oct 9, 2019

Agreed.

@mharms mharms deleted the feature/fMRI_spatial_coverage branch October 9, 2019 14:58
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants