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

remove fsl deps, containerization wip #12

Closed
wants to merge 16 commits into from

Conversation

bpinsard
Copy link

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.

@bpinsard bpinsard marked this pull request as ready for review August 30, 2023 14:59
Copy link

@effigies effigies left a 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.

Comment on lines 210 to 211
outputOrient = get_fslorient(self.input_nii)
if outputOrient == b"NEUROLOGICAL":

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Simpler:

Suggested change
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.

Copy link
Member

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.

gradunwarp/core/unwarp_resample.py Outdated Show resolved Hide resolved
bpinsard and others added 2 commits August 30, 2023 17:11
Co-authored-by: Chris Markiewicz <effigies@gmail.com>
Co-authored-by: Chris Markiewicz <effigies@gmail.com>
@coalsont
Copy link
Member

coalsont commented Dec 20, 2023

Some of this seems to have been incorporated into #15 or otherwise separated, closing due to merge conflicts.

@coalsont coalsont closed this Dec 20, 2023
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.

3 participants