-
Notifications
You must be signed in to change notification settings - Fork 56
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
use Image Orientation (Patient) attribute #26
Conversation
@carluri would you be able to take a look at this today? |
translated_contour_data = translate_contour_to_pixel_coordinants(reshaped_contour_data, series_slice) | ||
polygon = [np.array([translated_contour_data[:, :2]], dtype=np.int32)] | ||
translated_contour_data = apply_transformation_to_3d_points(reshaped_contour_data, transformation_matrix) | ||
polygon = [np.around([translated_contour_data[:, :2]]).astype(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.
This changes test IOUs from
0.95, 0.87, 0.85, 0.95
to
1.0, 0.96, 0.97, 1.0
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.
Amazing!
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.
Would you mind increasing their thresholds with the tests to reflect this?
rt_utils/image_helper.py
Outdated
@@ -22,7 +22,7 @@ def load_sorted_image_series(dicom_series_path: str): | |||
raise Exception("No DICOM Images found in input path") | |||
|
|||
# Sort slices in ascending order | |||
series_data.sort(key=lambda ds: ds.SliceLocation, reverse=False) | |||
series_data.sort(key=lambda ds: get_slice_position(ds), reverse=False) |
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.
SliceLocation no longer needed
tests/test_rtstruct_builder.py
Outdated
mask[10:70, 5:15, 0] = 1 | ||
mask[60:70, 5:40, 0] = 1 | ||
|
||
IOU_threshold = 0.95 # Expected accuracy |
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.
You mentioned some of the test IOUs went to 1.0. Can we expect an accuracy of 1.0 here as well?
translated_contour_data = translate_contour_to_pixel_coordinants(reshaped_contour_data, series_slice) | ||
polygon = [np.array([translated_contour_data[:, :2]], dtype=np.int32)] | ||
translated_contour_data = apply_transformation_to_3d_points(reshaped_contour_data, transformation_matrix) | ||
polygon = [np.around([translated_contour_data[:, :2]]).astype(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.
Would you mind increasing their thresholds with the tests to reflect this?
tests/test_rtstruct_builder.py
Outdated
@@ -164,6 +165,36 @@ def test_contour_data_sizes(new_rtstruct: RTStruct): | |||
# Then using approximation leads to less data within the contour data | |||
assert get_data_len_by_index(new_rtstruct, 0) < get_data_len_by_index(new_rtstruct, 1) | |||
|
|||
def test_load_sorted_series(oriented_series_path): |
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.
Curious, what are we testing here?
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.
It was checking if we can determine ImagePosition's of all the slices using ImageOrientation and ImagePosition's of the first and last slice.
Now that I think about it this test is unnecessary, because we check the correctness of ImageOrientation inside get_slice_directions anyway. Removed it in the next commit.
rt_utils/image_helper.py
Outdated
return mat | ||
|
||
def get_patient_to_pixel_transformation_matrix(series_data): | ||
return np.linalg.inv(get_pixel_to_patient_transformation_matrix(series_data)) |
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've never had a chance to work with matrices in practice. Since these are just linear transformations we won't have to worry about precision errors or un-invertible matrices right?
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.
Yup, these are affine transformations, so they will always have an inverse.
But this line was just me being lazy. Removed it in the next commit and calculated the inverse by hand, so we don't have to worry about numerical stability of the inv function.
return np.linalg.inv(get_pixel_to_patient_transformation_matrix(series_data)) | ||
|
||
def apply_transformation_to_3d_points(points: np.ndarray, transformation_matrix: np.ndarray): | ||
vec = np.concatenate((points, np.ones((points.shape[0], 1))), axis=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.
Would you mind leaving a comment explaining what is going on?
Awesome work @w0nsh! Really cool to see that my linear algebra courses actually came in handy. I've left a few comments and some requests for changes, only minor things really. It all looks good to me though I'm not the most familiar with matrix operations. Once my comments have been addressed, we can merge this along with #28 and create a new release 🎉 |
Needed to support DICOMs with different image orientations so I made some small modifications in image_helper.
If you'd like to merge it I can refactor the code a bit and add some tests.