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

Loading freesurfer surface mesh and CRAS offset #1101

Open
ymzayek opened this issue Apr 28, 2022 · 4 comments
Open

Loading freesurfer surface mesh and CRAS offset #1101

ymzayek opened this issue Apr 28, 2022 · 4 comments

Comments

@ymzayek
Copy link

ymzayek commented Apr 28, 2022

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
Copy link
Member

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
Copy link
Author

ymzayek commented Apr 29, 2022

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
Copy link
Member

effigies commented Apr 29, 2022

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
Copy link
Author

ymzayek commented Apr 29, 2022

Yes clear and I agree. Thanks again!

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

No branches or pull requests

2 participants