-
Notifications
You must be signed in to change notification settings - Fork 52
Fix DICOMSeriesToVolumeOperator casting bug #529
New issue
Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? # to your account
Fix DICOMSeriesToVolumeOperator casting bug #529
Conversation
@@ -112,7 +112,7 @@ def generate_voxel_data(self, series): | |||
# with the NumPy array returned from the ITK GetArrayViewFromImage on the image | |||
# loaded from the same DICOM series. | |||
vol_data = np.stack([s.get_pixel_array() for s in slices], axis=0) | |||
vol_data = vol_data.astype(np.int16) | |||
vol_data = vol_data.astype(np.uint16) |
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 can only be safe done when the pixel data is in fact unsigned, and the following DICOM tag tells the fact (when value == 1)
Tag (0028,0103)
Type Required (1)
Keyword PixelRepresentation
Value Multiplicity 1
Value Representation Unsigned Short (US)
Example Values
1
0
Enumerated Values:
0000H
unsigned integer.
0001H
2's complement
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.
Ah! You told me to do this a while ago, and it slipped my mind! I'll add that
if np.can_cast(vol_data, np.uint16, casting='safe'): | ||
vol_data = np.array(vol_data, dtype=np.uint16) | ||
else: | ||
vol_data = np.array(vol_data, dtype=np.int32) |
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.
I would not use np.int32 here. The slope and intercept both are decimals, so the vol_data in this block could well be np.float32. Notice also line 158 casted the vol_data to np.float64 (but the original code casts it back to int16 erroneously), as there could be float64 pixel values, so the can_cast check (to float32) will be a safe way.
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 is something i was wondering about... whether the pixel array values should always be ints. I simply guessed that all pixel array values would be ints, so any slope/intercept scaling returning floats should be rounded to ints. But that makes sense - i'll change to float32
Signed-off-by: will tepe <will.tepe@cchmc.org>
Signed-off-by: will tepe <will.tepe@cchmc.org>
d17c82f
to
f3f71f2
Compare
@@ -112,7 +112,8 @@ def generate_voxel_data(self, series): | |||
# with the NumPy array returned from the ITK GetArrayViewFromImage on the image | |||
# loaded from the same DICOM series. | |||
vol_data = np.stack([s.get_pixel_array() for s in slices], axis=0) | |||
vol_data = vol_data.astype(np.int16) | |||
if slices[0][0x0028,0x0103].value == 1: |
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.
value 0 is for unsigned integer.
|
There is one more commit with additional changes. The problem is we need the datatypes for the slope and intercept to be compatible with the volume data. uint16 and float32 are not compatible. We ended up needing to check the datatypes of all 3 (vol_data_, slope, intercept) in order to do any casting. Our solution is not elegant, but it will always find the lowest overhead datatype between the options of uint16, float32, and float64. There may be some benefit in checking again to see if the datatype can be further reduced after the slope/intercept transform is applied. We are open to discussion on this and could add that functionality as well if desired. |
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.
Looks good to me.
The array was being read in as int16 which caused some values to be truncated, it should instead be uint16. I also added a check that uint16 is sufficient after the slope/intercept transformation is applied. if it is not, it will use int32 instead.