search

nipy / nibabel

Python package to access a cacophony of neuro-imaging file formats
http://nipy.org/nibabel/
Other
653 stars 258 forks source link

Loading freesurfer surface mesh and CRAS offset #1101

Open ymzayek opened 2 years ago

ymzayek commented 2 years ago

Hello, this issue concerns this open pull request nilearn/nilearn#2215. In brief, it was pointed out that there is an offset observed between loaded surface mesh (extracted by Freesurfer) and volume data that is related to CRAS coordinates. I am wondering whether it is an issue to address on nilearn's side or nibabel's. This could concern the function nibabel.freesurfer.io.read_geometry not loading the coordinates correctly. Let me know what you think. Thanks!

effigies commented 2 years ago

The coordinates are presented as they are stored. FreeSurfer applies an additional offset stored in c_ras. I know both HCP pipelines and fMRIPrep have compensated for this.

https://github.com/Washington-University/HCPpipelines/blob/3a6406cc065227a45b147f2e08ce09f171de21df/PostFreeSurfer/scripts/FreeSurfer2CaretConvertAndRegisterNonlinear.sh#L158-L167 https://github.com/Washington-University/workbench/blob/19ae43ab3eadfbb97668dd0e7fe792d2372f409a/src/Algorithms/AlgorithmSurfaceApplyAffine.cxx#L73-L91

https://github.com/nipreps/niworkflows/blob/32a33d61ab64e262a4ebcf8afcf232d70c2f92b3/niworkflows/interfaces/surf.py#L570-L576

I can't remember how HCP does it, but fMRIPrep applies the translation directly to the coordinates and then zeros out the translation. But this is just in the GIFTI translations.

ymzayek commented 2 years ago

Ok I see, thanks for the feedback. There was already a solution contributed to nilearn that was never finished so I will pick it up from there, just wanted to verify that for nibabel this is intentionally done in terms of how the coordinates are loaded for these surfaces.

effigies commented 2 years ago

I'd say it's more-or-less intentional, and we certainly can't change it now without potentially breaking people's pipelines. We could add a parameter like (apply_cras which defaults to False) to allow users to select a new behavior, though.

But from a development perspective, I wouldn't want to force users to use the absolute latest version of nibabel to use the latest nilearn (what if we have a bug that some other dependency needs to pin the version around?); you want to wait a while for a feature to have been around before depending on it. So whether or not we add some feature to nibabel, you probably want to go ahead with the nilearn-side fix for the time being.

ymzayek commented 2 years ago

Yes clear and I agree. Thanks again!