-
Notifications
You must be signed in to change notification settings - Fork 29
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
remove fsl deps, containerization wip #12
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A couple notes on the replacements for FSL utils that don't depend on NIfTI-specifics.
TBH I would try to break this out into separate PRs. There's a lot going on here, with reformatting, removing the extensions and tests, replacing FSL and adding a Dockerfile.
gradunwarp/core/unwarp_resample.py
Outdated
outputOrient = get_fslorient(self.input_nii) | ||
if outputOrient == b"NEUROLOGICAL": |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Simpler:
outputOrient = get_fslorient(self.input_nii) | |
if outputOrient == b"NEUROLOGICAL": | |
if np.linalg.det(self.input_nii.affine) > 0: |
You don't really care about all the logic of consistent or inconsistent or unknown. You just need the affine according to the NIfTI 3 methods, which both FSL and nibabel implement. And this also doesn't tie you to NIfTI. It will work just fine with an MGH file.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It might be cleaner to restrict it to the 3x3 determinant - the 4th row being 0 0 0 1 (as it always should be, but we don't check that here) should cause it to ignore the translation values, but I prefer not to rely on assumptions when I don't have to.
Co-authored-by: Chris Markiewicz <[email protected]>
Co-authored-by: Chris Markiewicz <[email protected]>
Some of this seems to have been incorporated into #15 or otherwise separated, closing due to merge conflicts. |
FSL was only used for fslorient / fsval which I believe can be handled with nibabel.
Removing it would make the package pure-python (with the non-necessary c-extension removed earlier too #9 ).
Maybe if someone with knowledge about nifti orientation can double-check that the changes makes sense, that would be great.