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

FSL dependency #482

Closed
mb3152 opened this issue Oct 24, 2022 · 18 comments · Fixed by #498
Closed

FSL dependency #482

mb3152 opened this issue Oct 24, 2022 · 18 comments · Fixed by #498
Assignees

Comments

@mb3152
Copy link

mb3152 commented Oct 24, 2022

I would like to remove the FSL dependency from QSIPrep.

@mb3152 mb3152 self-assigned this Oct 24, 2022
@mattcieslak
Copy link
Collaborator

Can you open up a PR and enable contributions from other editors? I think with TORTOISE included we can remove it without loss of performance

@mb3152
Copy link
Author

mb3152 commented Oct 25, 2022

Got it, @jbh1091 is working on it for now.

@jbh1091
Copy link
Collaborator

jbh1091 commented Oct 25, 2022

@mattcieslak I'm looking at FAST usage right now. I basically see 2 options for replacement: Freesurfer segmentation and AFNI's 3dSeg. Both have differences that prevent a quick plug-and-play swap (no CSF mask in Freesurfer, no TPMs in AFNI). Did you have a solution in mind?

@mb3152 mentioned FAST outputs are already not used much, so deleting the current usage and added segmentation references later in the pipeline when needed is also an option.

@mattcieslak
Copy link
Collaborator

We can remove FAST from preprocessing entirely. Nothing in the preprocessing workflow uses a brain segmentation other than distortion correction plotting. Maybe we could use Atropos from ANTs, or 3dSeg sounds interesting too.

The trickiest thing to replace is going to be avscale, which we use to convert itk transforms to FSL transforms and from those calculate the head motion parameters.

I have no idea how FSL calculates these motion parameters. They're different from what you get if you read the itk transform files as Euler3DTransforms and get the translations and rotations.

@jbh1091
Copy link
Collaborator

jbh1091 commented Oct 27, 2022

Totally separate question: I know TORTOISE will be replacing eddy and topup; does that replacement extend to FUGUE and PRELUDE as well? From what I've been reading they do similar things (EPI distortion correction) just with a different method.

@mattcieslak
Copy link
Collaborator

These also require different fieldmap acquisitions. AFAIK there is nothing out there that does exactly what fugue and prelude do, but unless you acquire GRE fieldmaps (not as popular nowadays) these programs are not useful

@jbh1091
Copy link
Collaborator

jbh1091 commented Oct 27, 2022

OK that makes sense, thanks. If there isn't a good replacement but it's also not used much, I'll just plan to remove them entirely.

@jbh1091
Copy link
Collaborator

jbh1091 commented Oct 28, 2022

What push/review strategy makes the most sense to you? There are a ton of changes to make, so it may be better for me to push to the branch after I change (and locally test) each component individually. A piecewise approach seems easier to write and easier to review.

@mb3152
Copy link
Author

mb3152 commented Nov 17, 2022

@mattcieslak can we use Matthew Brett's code for the avscale?

https://github.com/matthew-brett/transforms3d/blob/main/transforms3d/affines.py

@mattcieslak
Copy link
Collaborator

If you can show that it finds the same values as avscale then great, but I've not found anything (including ITK) that produces the same values as avscale

@mb3152
Copy link
Author

mb3152 commented Nov 21, 2022

Great, we are testing it now. Also, fsl.merge and fsl.splitare only used in https://github.com/PennLINC/qsiprep/blob/master/qsiprep/interfaces/images.py which looks specific to FSL, mcflirt, ie stuff we won't run. Is that true?

@mattcieslak
Copy link
Collaborator

Sounds right. You can replace fsl's merge with a nilearn concat_imgs. This happens in a lot of places in the code

@jbh1091
Copy link
Collaborator

jbh1091 commented Nov 29, 2022

Hey @mattcieslak , I did some testing of the transforms3d package Max linked above. Using the 4x4 affine from the temporary _FSL.xfm file created by calling c3d_affine_tool ras2fsl, here are my results as they compare to avscale:

edit: attached spreadsheet because my text formatting got erased

avscale transform3d
scale/zoom 1.030486 1.017067 1.042259 1.0304864 1.01706744 1.04225888
shear 0.000629 -0.000471 0.009042 0.00062085 -0.00047679 0.00926616
rotation -0.008436 0.052675 0.037594 n/a n/a n/a (could compute from rot. matrix)
translation 1.851074 0.586667 1.297234 -1.21915 -5.77937 -1.1296

Rmat 0.997907 -0.037533 0.052651 0.99790739 -0.03753295 0.05265085
0.038028 0.999241 -0.008424 0.03802758 0.99924118 -0.00842405
-0.052295 0.010409 0.998577 -0.05229472 0.01040861 0.99857745

So everything is very similar except for the translations which are way off. My first thought is that maybe this is due to the orientation change applied by c3d_affine_tool. Any thoughts?

Based on a comment here (https://github.com/ANTsX/ANTs/wiki/ITK-affine-transform-conversion) ANTs is spitting out LPS affines, so I'm confused about the ras2fsl usage as that implies the transform is converted from LPS to RAS at some point that I'm not seeing. No idea what orientation ras2fsl is converting to either.

avscale_vs_transforms3d.xlsx

@jbh1091 jbh1091 linked a pull request Dec 7, 2022 that will close this issue
@jbh1091
Copy link
Collaborator

jbh1091 commented Dec 12, 2022

In workflows/anatomical.py, I replaced FSL usage in init_skullstrip_afni_wf with AFNI's 3dSkullstrip. This is currently causing an error in circleci due to a missing library. I found a potential fix on the AFNI forums, but it also appears that this workflow is never used outside of debugging (i.e. not in actual usage of qsiprep).

I'm proposing removing the workflow call such that the equivalent (and functioning) ANTs workflow is always called instead.

@mattcieslak
Copy link
Collaborator

mattcieslak commented Dec 12, 2022

Hi @jbh1091 if you got the rotations to match with the affine library, maybe it's possible to get the translation out of simpleitk?

The c3d nomenclature is also opaque to me, which is shy avscale was left in :(.

I think we can remove the afni workflow and replace it with one that uses synthstrip for skull stripping. The ants brain extraction workflow can take an hour or more, so we needed an alternative that can run quickly during CI tests. synthstrip works for this and is probably the way to go

@jbh1091
Copy link
Collaborator

jbh1091 commented Dec 14, 2022

Cool, I'll explore using simpleitk to extract the translations. As long as we eventually get all the same numbers as avscale, it shouldn't be a problem to keep treating c3d as a black box.

I think synthstrip as a replacement is working now for CI purposes.

Max and I were also testing out using AFNI's 3dTsplit4D as a replacement for FSL Split for the SplitDWIs node in interfaces/images.py, but we are running into a problem. There is no pre-written node interface for this particular function, so we were trying to use subprocess.Popen instead, following your usage elsewhere in qsiprep as a template. But we keep getting a FileNotFound error in the command line call for some reason. The input file definitely exists and appears to be passed correctly to both the workflow and the command line call, but it seems like maybe the shell can't see the folder the input file is in? Would really appreciate any suggestions there.

@jbh1091
Copy link
Collaborator

jbh1091 commented Dec 16, 2022

Hi @mattcieslak ,
Update on 3dTsplit4D: I changed tack from trying to use a command line call and instead write a new interface for it, using pre-existing nipype interfaces as templates. I then reworked the workflow connections and got the following error:
Cannot run qsiprep. Missing dependencies:
3dTsplit4D (Interface: TSplit)

This is similar to the errors we saw when trying to use subprocess.call() and os.system() as alternatives for Popen when that wasn't working. Is it possible that this AFNI function is not in the qsiprep build?

Edit: Found the offending line in the qsiprep_build dockerfile. Adding 3dTsplit4D to the list of AFNI functions specifically kept should solve the issue. Currently have a relatively complex node-based implementation that should work, but can easily revert to a simpler command line call if needed. Can't test until qsiprep_build is updated.

@jbh1091
Copy link
Collaborator

jbh1091 commented Dec 28, 2022

I now have a (probably) working replacement for avscale. Not sure how to add the transforms3d package to the container to test though. Once I can test my replacement, I'll re-add my SplitDWIs fix which will only work with an updated qsiprep-build (due to currently missing 3dTsplit4D). I'll write up a log of all the changes I made in the new year so you can more easily evaluate what works and what might cause a problem. I'll also document what changes will need to be made on your end with qsiprep-build. Have a happy new year!

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 a pull request may close this issue.

3 participants