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

SimpleITK version requirement #32

Closed
lassoan opened this issue Nov 29, 2022 · 16 comments
Closed

SimpleITK version requirement #32

lassoan opened this issue Nov 29, 2022 · 16 comments

Comments

@lassoan
Copy link
Contributor

lassoan commented Nov 29, 2022

First of all, congratulations on this package it works amazingly well. We've created a plugin for 3D Slicer that allows any users easily run it on their computer (can be installed by a few clicks).

We only encountered one issue using the package: its SimpleITK requirement is set to a very specific version that makes the package incompatible with almost every other packages that require a recent SimpleITK version.

Please relax the SimpleITK requirement and just require some minimum version.

I understand that the workaround for hardcoding a specific SimpleITK version was added because some invalid NIFTI files could not be loaded. However, do not give in, do not let the terrible NIFTI file format dictate such requirements.

@pieper
Copy link

pieper commented Nov 30, 2022

@wasserth also note that we have been working with the ITK devs to improve interoperabiility (e.g. here) so any issues can hopefully be fixed as part of the main line of ITK.

p.s. I agree with @lassoan: 💯 for making this cool tool available.

@wasserth
Copy link
Owner

I would like to remove the SimpleITK version restriction. However, at the moment this issue (SimpleITK/SimpleITK#1433) is still existing in the most recent version (v 2.2.0) if I understand it correctly. Unfortunately this error occurs quite regularly. So if I lift the restriction a lot of people will write me that TotalSegmentator is crashing with some error. Therefore I am hesitant to change it at the moment.

@lassoan
Copy link
Contributor Author

lassoan commented Nov 30, 2022

Would you consider adding support for NRRD file format for accepting input data and generating output?

Out of all the 3D medical image file formats, only NIFTI is so problematic: it has lots of incompatibility issues, inconsistency of image orientation in different software, failures to load in some software, etc. We (the 3D Slicer development team) on average spends a few hours per week with issues coming from NIFTI, while all the other formats are essentially problem-free. As you can imagine, this is really frustrating, because we could use this time for working on more useful things. If you encourage users to use NIFTI (by making it the default or only supported file format) then, you may need to spend considerable time with supporting users with their random NIFTI issues.

@wasserth
Copy link
Owner

I agree that making those file formats work is frustrating and takes quite a bit of time. However, in my experience the vast majority of people in the medical image machine learning community is using nifti files. NRRD is a lot less common. Also the main tools I am working with only work with nifti (nnUNet, Nora imaging platform). So unfortunately there is no way around nifti for now.
What python library do you recommend for loading NRRD files?

@lassoan
Copy link
Contributor Author

lassoan commented Nov 30, 2022

pynrrd works well for reading/writing nrrd files.

There have been some initiatives to add nrrd support to nibabel (see here). Maybe the simplest would be to make that happen, because most packages just use nibabel to read/write 3D images, and having nrrd support directly in nibabel would automatically make nrrd images usable in all those packages.

@wasserth
Copy link
Owner

Thank you. I will have a look at it. However, for the near future I will stay with nifti. Maybe the easiest solution for you is to create a branch of totalsegmentator where you relax the SimpleITK version requirement and then use this branch for the Slicer plugin.

@kmordhwaj
Copy link

kmordhwaj commented Dec 31, 2022

I am facing same issue when I am inside my virtual env. I have to pip install TotalSegmentator here again. But now it's giving me SimpleITK==2.0.2 compatibility error.
Please help @wasserth

Okay. I got a solution.
Downgraded to python version 3.10.2
Now it's not giving error on installation.

@lassoan
Copy link
Contributor Author

lassoan commented Dec 31, 2022

This SimpleITK version requirement should be simply removed. There are some invalid nifti files that certain SimpleITK versions reject to load, but that should not be a reason to give all TotalSegmentator users headaches.

We had to use an ugly workaround (install TotalSegmentator without dependencies then edit its requirements file and install dependcies manually - see some more details here), which is not fair. Those users should be forced to implement workarounds that want to work with invalid nifti files.

@wasserth
Copy link
Owner

wasserth commented Jan 3, 2023

Unfortunately the "invalid" nifti files occur quite often. When I get a batch of data from our PACS and convert them to nifti, then it typically contains a least one file with this problem. Not sure how this happens. Maybe the conversion with dcm2niix is having some problems.
Nevertheless I just committed a change where I removed the SimpleITK version requirement. Then it is more flexible. People with such files will have to install SimpleITK==2.0.2 themselves then.

@lassoan
Copy link
Contributor Author

lassoan commented Jan 3, 2023

Awesome, thank you!

Dcm2niix developer Chris Rorden is very knowledgeable and responsive, so if you report the issue then he can surely fix it very quickly. Maybe it is already fixed or there are some options you can use to avoid the error.

@lassoan
Copy link
Contributor Author

lassoan commented Jan 3, 2023

Is the change available in a released TotalSegmentator version? (git+https requires users to have git installed, which is not the case for many non-progtammer users)

@wasserth
Copy link
Owner

wasserth commented Jan 3, 2023

It is not yet part of a release.
You could install the master branch without git the following way:

pip install https://github.com/wasserth/TotalSegmentator/archive/master.zip

lassoan added a commit to lassoan/SlicerTotalSegmentator that referenced this issue Jan 6, 2023
SimpleITK version downgrade has been removed from TotalSegmentator (see wasserth/TotalSegmentator#32). Therefore, the custom installation workaround (install without dependencies and then install dependencies manually except SimpleITK) is not needed anymore.

Use github .zip file instead of git+https to avoid requiring git installation.
@lassoan
Copy link
Contributor Author

lassoan commented Jan 6, 2023

Thank you @wasserth - installing the latest TotalSegmentator from https://github.com/wasserth/TotalSegmentator/archive/master.zip works very well. No need for git and no need for the ugly SImpleITK installation hack anymore.

@gdevenyi
Copy link

gdevenyi commented Apr 5, 2023

@wasserth

Unfortunately the "invalid" nifti files occur quite often. When I get a batch of data from our PACS and convert them to nifti, then it typically contains a least one file with this problem. Not sure how this happens. Maybe the conversion with dcm2niix is having some problems.

Your techs are rotating the slice stack. Have them reposition the patient if they can't fit in the FOV, or make the FOV larger. (Slice stack rotation means the voxel slicing for the data no longer aligns with the scanner coordinates, causing the odd direction cosines)

... Sorry for the drive-by comment.

@wasserth
Copy link
Owner

wasserth commented Apr 5, 2023

Thanks for pointing this out. This probably is the reason. Unfortunately, I can not really make our techs change their approach. I guess this issue occurs in a lot of radiology departments. SimpleITK should have a way to deal with those images instead of simply failing.

@pieper
Copy link

pieper commented Apr 5, 2023

Right, I don't think this issue was due to the scans, but to the processing steps between dicom and nifti that lead to issues later with ITK being able to read the data. The issue here is closed because I understand all those issues have been resolved.

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

No branches or pull requests

5 participants