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?
| 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.
| 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
There was a problem hiding this comment.
This branch covers Minc1Image and Minc2Image, as the dtypes are all supported and do not need to be remapped. Would elif image_class in [nib.Minc1Image, nib.Minc2Image] be preferable as it is more explicit?
There was a problem hiding this comment.
Ah okay, that's fine, I misunderstood
| 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?
There was a problem hiding this comment.
Ok, I can factor a few of these out from the sitk and itk tests as well. I believe some of them may be in the original volume tests as well.
There was a problem hiding this comment.
Should I also update these in this PR or keep it focused on purely nibabel and standardize these changes in a followup PR after all of the initial external dependency PRs are merged?
There was a problem hiding this comment.
I think this PR is fine unless you think that will create problems
Co-authored-by: Chris Bridge <chrisbridge44@googlemail.com>
Co-authored-by: Chris Bridge <chrisbridge44@googlemail.com>
Co-authored-by: Chris Bridge <chrisbridge44@googlemail.com>
Co-authored-by: Chris Bridge <chrisbridge44@googlemail.com>
Co-authored-by: Chris Bridge <chrisbridge44@googlemail.com>
2 participants
No description provided.