-
Notifications
You must be signed in to change notification settings - Fork 60
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
vol.load_dicom() implementation #30
Conversation
The parenthesis in the macro UPDATE and INTERPOLATE are causing problems when compiling on windows. See arcadelab#26 for more details.
The computation of lps_from_ijk turned out to be harder than anticipated. Reason for that is the immensely complex and horribly covered dicom standard. Trying to use nibabel next to outsource all the error handling to another lib.
Using nibabel.nicom.dicomwrappers.MultiframeWrapper to construct the affine transform from the available dicom tags. Evidently this is complex enough to _not_ maintain it in this project. See this post to get a feeling for the complexity: https://nipy.org/nibabel/dicom/dicom_orientation.html#defining-the-dicom-orientation. The used class warns, that the exposed functionality is not well tested. I will verify that it works with our datasets.
Using nibabel.nicom.dicomwrappers.MultiframeWrapper seems not as well fitting as expected. The class was conceptualized for phillips dicoms and there seems to be a non-trivial difference. Decided to implement the affine transform computatio from scratch following these resources: https://nipy.org/nibabel/dicom/dicom_orientation.html#defining-the-dicom-orientation https://mrohleder.medium.com/coordinate-systems-in-medical-data-science-a-rant-90394f60b27 https://github.com/nipy/nibabel/blob/master/nibabel/nicom/dicomwrappers.py
Same number of parameters as from_nifti() now.
BUG: When reading in the Simply accommodating this difference in the affine transform (as the current implementation does) does not suffice, as the projector later tries to access the data in Will fix this soon. |
The difference in indexing between dicom and this framework were previously dealt with in the affine matrix by switching the columns of i and j. Now, we change the internal layout of the supplied volume prior after loading and leave the affine transform untouched.
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.
Tested this version with a Spin Volume. The generated angles have to be adapted to rotate around the z-axis.
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.
This looks great! Thanks so much for implementing this. I'm just going to refer to @mmjudish to look at the parentheses in the CUDA, as he's currently working on that section.
Grouped the dicom tags by per-frame and per-slice attributes for better readability. Removed not needed InvalidDicomError import.
The parentheses in the CUDA kernel are a different topic and should be dealt with in a seperate thread. Removing that commit from the history of this branch. This reverts commit 8f45483.
I removed the changes in the CUDA kernel from this branch as they are not related to the @mmjudish for the parenthesis issue, please refer to the other open pull request :) |
Hey there! As discussed, I started working on the
vol.load_dicom()
feature.It now works for the datasets I work with, but extracting the affine transform from DICOM metadata is no easy feat. There are whole specialized libraries trying to provide an easy nifti-like interface to DICOM images. See this issue for example.
In this draft-pull-request, I offer you to integrate the solution that I tested against MultiFrame-Dicom volumes from a Siemens Cios Spin-System.
From a design point-of-view it might be more sensible to outsource the task to a library that specializes on this. For example nibabel or medio.
As you can see in this commit, I already tried to use the
nibabel.nicom.dicomwrappers.MultiframeWrapperclass
. With a test dataset I used, It failed to correctly calculate the lps_from_ijk transform. However, in the thread about this issue it sounds like we can expect an update with this functionality.Kind regards,
Max