Add nibabel volume conversions and tests#411
Conversation
| ' Casting to float64 is not possible.' | ||
| ) | ||
|
|
||
| nifti = nib.Nifti1Image( |
There was a problem hiding this comment.
nibabel has several other image classes that I believe could represent a volume, listed here. Not all of them are relevant, but the MINC and MGH formats seem worth consideration. I think at the very least, we should support both Nifti1Image and Nifti2Image, meaning the to_nib method should already be changed to allow the user to specify the class to which they wish to convert. Additionally, several of these classes can accept an array, affine, and dtype exactly as the Nifti1Image class or with extremely minor changes.
However, I do not think all of the formats support the same data types. Are you interested in supporting other formats @CPBridge? If so, would it be best to group different classes into separate to_* methods or have any branching logic be included in a single to_nib method?
| ] | ||
| itk = ["itk>=5.4.0"] | ||
| sitk = ["SimpleITK>=2.2.1"] | ||
| nib = ["nibabel>=4.0.0"] |
There was a problem hiding this comment.
I think it would be clearer to use the full name
| nib = ["nibabel>=4.0.0"] | |
| nibabel = ["nibabel>=4.0.0"] |
| image_class: Literal[ | ||
| 'Nifti1Image', | ||
| 'Nifti2Image', | ||
| 'MGHImage', | ||
| 'Minc1Image', | ||
| 'Minc2Image', | ||
| 'AnalyzeImage' | ||
| ] = 'Nifti1Image' |
There was a problem hiding this comment.
Elsewhere in the library, we use enum.Enums for a defined set of options. I think it would be better to stick to that convention here. Literal is only useful for type checking. It does nothing at runtime.
| The Volume is converted to one of several 3D Image classes ( | ||
| ``nibabel.Nifti1Image``, ``nibabel.Nifti2Image``, ``nibabel.MGHImage``, | ||
| ``nibabel.Minc1Image``, ``nibabel.Minc2Image``, ``nibabel.AnalyzeImage`` | ||
| ), defaulting to ``nibabel.Nifti1Image``. If its array'current datatype |
There was a problem hiding this comment.
Typo
| ), defaulting to ``nibabel.Nifti1Image``. If its array'current datatype | |
| ), defaulting to ``nibabel.Nifti1Image``. If its array's current datatype |
| self = self.astype(np.uint8) | ||
|
|
||
| dtype = self.dtype | ||
| image_class = getattr(nib, image_class) |
There was a problem hiding this comment.
I would guard this with a check on the values, and issue a more helpful error message if an inappropriate value is passed. As noted above, this would probably be better handled with an enum (there are examples of this all over the library, for example the segmentation_type parameter of the Segmentation constructor)
| else: | ||
| dtype_map = {} |
There was a problem hiding this comment.
This will just cause confusion downstream I would think
| Returns | ||
| ------- | ||
| highdicom.Volume: | ||
| Volume constructed from the `itk.Image`. |
There was a problem hiding this comment.
| Volume constructed from the `itk.Image`. | |
| Volume constructed from the Nibabel image. |
| @classmethod | ||
| def from_nibabel( | ||
| cls, | ||
| nib_im: 'nibabel.spatialimages.SpatialImage', # noqa: F821 |
There was a problem hiding this comment.
We have generally been verbose with parameter names
| nib_im: 'nibabel.spatialimages.SpatialImage', # noqa: F821 | |
| nibabel_image: 'nibabel.spatialimages.SpatialImage', # noqa: F821 |
| Raises | ||
| ------ | ||
| ValueError | ||
| When the volume is not 3D (multiple channels are unsupported). |
There was a problem hiding this comment.
It's not a volume at this stage
| When the volume is not 3D (multiple channels are unsupported). | |
| When the input image is not 3D (multiple channels are unsupported). |
| def read_multiframe_ct_volume(): | ||
| im = imread(get_testdata_file('eCT_Supplemental.dcm')) | ||
| return im.get_volume() |
There was a problem hiding this comment.
There's some repetition in some of these functions I think. It might make sense to factor these out into the tests/utils.py file?
2 participants
No description provided.