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

ENH: insert the gradunwarp workflow in anat template workflow #355

Draft
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

bpinsard
Copy link
Contributor

@bpinsard bpinsard commented Aug 2, 2023

Working on nipreps/fmriprep#1550.
Relies on an updated version of the gradunwarp https://github.com/bpinsard/gradunwarp/tree/fix/mem_leaks.

Unwarping is inserted in the anat_template_wf and is thus applied to T1w and T2w.
It is applied after denoising to avoid messing with noise structure with interpolation.
image
Does it make sense?

Necessary PRs:
nipreps/niworkflows#819

@bpinsard bpinsard changed the title Enh/gradunwarp ENH: insert the gradunwarp workflow in anat template workflow Aug 2, 2023
@effigies
Copy link
Member

effigies commented Aug 2, 2023

Cool! I suspect that we can't use FSL warps directly with antsApplyTransforms. @oesteban can nitransforms translate these yet?

Some quick googling shows the SCT folks have the transform going the other way, which we could look at if we don't already support this: spinalcordtoolbox/spinalcordtoolbox#2529

@bpinsard
Copy link
Contributor Author

bpinsard commented Aug 2, 2023

Right I wrongly assumed these were compatible. Regarding the transforms, I think it would be possible to make gradunwarp output in a nitransforms compatible format maybe skipping/simplifying that step.

@oesteban
Copy link
Member

oesteban commented Aug 2, 2023

can nitransforms translate these yet?

It would be experimental, but the implementation is there mostly thanks to @smoia :)

@smoia
Copy link

smoia commented Aug 2, 2023

Glad to hear I did something useful in my life - I'm not sure I did though. When I was trying not to break nitransforms, I was working with FSL 5.x transformations, but FSL 6.x transforms were breaking the code. I did open an issue about it, but can't find it now...
Also happy to chime in again though!

@bpinsard
Copy link
Contributor Author

bpinsard commented Aug 4, 2023

I upgraded the workflow in niworkflow to convert the warp from fsl to itk, based on the code of nitransforms. As nitransforms.io only inputs transforms, I inverted the itk input function to output in itk format (flipping sign of the 1st and 2nd dimensions) and from what I understand itk deformation fields are relative. It seems to warp in a sensible way, though the gradient correction transforms are very subtle.

sMRIPrep doesn't use the warp field as it directly pass the corrected image in the anat template workflow so that's only used in fMRIPrep when combining all transforms for one-step interpolation.

Side question: do you have specific datasets you use for testing fmriprep with sdcflows? (small datasets with different types of fieldmap) I noticed that sdcflows ones are generated on the fly during testing, but I suppose you have integration tests that do not run on github actions.

Thanks for all the feedback!

@oesteban
Copy link
Member

oesteban commented Aug 7, 2023

FSL 6.x transforms were breaking the code.

You left it working for FSL 5. Then I realized that FSL 6 started to break all tests across the board and discovered a nasty bug in their interpolation. I reported the bug to FSL, and they must have fixed it (it wasn't the most transparent process tbh).

See? you did something :)

@bpinsard, @effigies - I don't know if in this PR or a separate one, but I would like to see sMRIPrep pick up corrected files when available.

I don't know this is the PR where that logic would go in (why not?), but if it helps, we are calling our T1w as follows:

sub-pilot_ses-16_acq-original_T1w.nii.gz
sub-pilot_ses-16_acq-undistorted_T1w.nii.gz

@effigies effigies added this to the 0.16.0 milestone Mar 22, 2024
@mattcieslak
Copy link

Is there any interest in using the command from TORTOISE that directly creates an ITK displacement field from coef files? It works for all scanner manufacturers and has a permissive license

@bpinsard
Copy link
Contributor Author

bpinsard commented May 6, 2024

@mattcieslak thanks for this information.

When you say displacement field from coeffs files you mean as a replacement to gradunwarp?
Do you have a link to the documentation of the tool in TORTOISE which allows doing that?

I plan to work on this feature during the upcoming nipreps sprint.
I am not think that adding a new binary dependency would be ideal, but better have all the info to make that decision.

Thanks!

@mattcieslak
Copy link

@bpinsard here is the code. I would be interested in helping out on this during the nipreps sprint too

@oesteban
Copy link
Member

oesteban commented May 7, 2024

Is there any interest in using the command from TORTOISE that directly creates an ITK displacement field from coef files? It works for all scanner manufacturers and has a permissive license

This should be implemented and functional in nitransforms already, and the dependency is already included. As an additional perk, it makes the transform compatible with BIDS' X5 file format.

@effigies
Copy link
Member

effigies commented May 7, 2024

I'm pretty sure we haven't implemented gradient coefficients in nitransforms. It should be possible to adapt gradunwarp, though.

@bpinsard bpinsard force-pushed the enh/gradunwarp branch 2 times, most recently from 9f37e54 to bdd7861 Compare May 16, 2024 18:36
@bpinsard
Copy link
Contributor Author

ok, rebase was way too difficult after fit/apply refactor, restarted from scratch.
There are added changes to pass BIDSLayout to get per-contrast/per-file metadata indicative of scanner distortion correction.
If all files distortion metadata for one contrast (T1w/T2w) are not consistent, it raises a RuntimeError to avoid any kind of crazy branching where a set of files have to be corrected while the other do not.

@bpinsard
Copy link
Contributor Author

Giving it more thought, I think the step should be inserted after denoising but before conform to keep the affine intact.

…fore conform to keep scanner-relative affines intact
@bpinsard
Copy link
Contributor Author

the failing CI will require niworkflows PR to be merged.

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.

5 participants