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

[WIP] ENH: 482 remove fsl dependency #498

Merged
merged 104 commits into from Apr 28, 2023
Merged

[WIP] ENH: 482 remove fsl dependency #498

merged 104 commits into from Apr 28, 2023

Conversation

jbh1091
Copy link
Collaborator

@jbh1091 jbh1091 commented Dec 7, 2022

Changes proposed in this pull request

Replacement of FSL tools when replacements are available, for example replacing BET with ANTs BrainExtraction. For FSL tools with no viable replacements (mainly FUGUE and PRELUDE) we add a check for FSL environment variables such that containers without FSL can't run them, and those who still want to use FSL are able to. A separate container without FSL will be generated to give users a choice between qsiprep installations with or without FSL installed.

Documentation that should be reviewed

Boilerplate and references require updating to match the removed or replaced FSL functions.

@jbh1091 jbh1091 linked an issue Dec 7, 2022 that may be closed by this pull request
@jbh1091
Copy link
Collaborator Author

jbh1091 commented Apr 21, 2023

Ready for re-review of remaining changes (mostly FAST removal related) when you are @mattcieslak

@mattcieslak
Copy link
Collaborator

I noticed that you removed t1_seg from a lot of these workflows. With #553 the t1_seg will be available no matter what, so I'm thinking it would make sense to let the tests fail on this build but keep t1_seg the way it was before in the dwi workflows.

If you revert those, we can merge this into master and then I'll merge it into #553. wdyt?

@jbh1091
Copy link
Collaborator Author

jbh1091 commented Apr 24, 2023

Good point, that works for me! I'll let you know when it's put back in. Ok to still leave out the TPMs or should those be added back as well?

@mattcieslak
Copy link
Collaborator

Awesome! The TPMs are never used, so those can stay out

@jbh1091
Copy link
Collaborator Author

jbh1091 commented Apr 24, 2023

Ok, I added all the segmentation node inputs back in. Things to note:
-FAST is still out, meaning the workflows in anatomical.py will fail due to lack of connections since the t1_seg node just isn't there. Any run would fail from a workflow error. Happy to add it back for the brief period in between merging this branch and your refactor
-Removed my addition of T1 atropos in workflows/fieldmap/pepolar.py to return to the original workflow that included seg
-Added back MNI seg nodes as well as t1 seg
-Did not change my removals of FAST options in MRTRIX workflows

Let me know if any other changes are necessary.

@mattcieslak
Copy link
Collaborator

It worked?!? If you can run this on a real test dataset and the results look good, I'll go ahead and merge!

@mb3152
Copy link

mb3152 commented Apr 28, 2023

Screenshot 2023-04-20 at 2 53 51 PM

This was from about a week ago with Joey's changes, not your most recent ones and fixing the connections, comparing it to the unstable from about a month ago. Mind merging and we can double check the outputs from the merge?

@mattcieslak
Copy link
Collaborator

The most recent version of the master branch is already included here, so if this branch works it will become the master branch when we merge it

@mb3152
Copy link

mb3152 commented Apr 28, 2023

awesome!! letting Joey hit the final button...

@jbh1091 jbh1091 merged commit af71e95 into master Apr 28, 2023
2 checks passed
@mattcieslak mattcieslak deleted the 482-fsl-dependency branch July 28, 2023 15:59
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.

FSL dependency
3 participants