-
Notifications
You must be signed in to change notification settings - Fork 42
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
[DAR-5588][External] Apply automated triaxial re-orientation of non-NifTI annotations for import #1008
base: master
Are you sure you want to change the base?
Conversation
`LPI` will result in axial re-ordering for coronally and sagitally oriented files: | ||
- Axially acquired files axial codes are ordered: L/R, P/A, I/S | ||
- Coronally acquired files axial codes are ordered: L/R, I/S, P/A | ||
- Sagittally acquired files axial codes are ordered: I/S, L/R, P/A |
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 true for DICOM, but for Nifti if we are using Nibabel to decide the primary plane, I'm not sure that this is enforced to be true (this was the diff in that other PR on orig implementation vs. nibabel on deciding what hte primary plane was for Niftis)
Rather than
if primary_plane == "CORONAL":
axial_flips[0], axial_flips[1], axial_flips[2] = (
axial_flips[0],
axial_flips[2],
axial_flips[1],
)
elif primary_plane == "SAGITTAL":
axial_flips[0], axial_flips[1], axial_flips[2] = (
axial_flips[2],
axial_flips[0],
axial_flips[1],
)
It might be safer to use the first column values of the nib.orientations.ornt_transform(original_ornt, ornt)
@@ -244,6 +244,116 @@ def get_sub(self, annotation_type: str) -> Optional[SubAnnotation]: | |||
return sub | |||
return None | |||
|
|||
def _flip_annotation_in_x_or_y(self, axis_limit: int, axis: CartesianAxis): |
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 deduplicate this and remove all the if else statements. One way to do it is to do two fns since we already decide on what axis is used based on an if/else when it is used in _apply_axial_flips_to_annotations
:
_flip_annotation_in_x
_flip_annotation_in_y
and those both call a shared helper _flip_annotation_coordinates
that just changes to either use key "x" or "y" based on which of _flip_annotation_in_x
/_flip_annotation_in_y
is calling it
for node in self.data["nodes"]: | ||
node["y"] = axis_limit - node["y"] | ||
|
||
def _flip_raster_layer_in_x_or_y( |
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.
Maybe you can put the raster logic as another case you are matching against in flip_annotation_coords/flip_annotation_coords
helper rather than having a separate method? and in that case you call the logic from another helper to declutter as well.
@@ -1567,3 +1677,9 @@ class StorageKeyDictModel(BaseModel): | |||
|
|||
class StorageKeyListModel(BaseModel): | |||
storage_keys: List[str] | |||
|
|||
|
|||
class CartesianAxis(Enum): |
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.
If take approach suggested above, can remove
return parsed_files | ||
|
||
|
||
def _flip_annotation_in_z( |
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 should maybe be a method of Annotation as well?
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'll make it a method of VideoAnnotation
axial_flips[0], | ||
axial_flips[1], | ||
) | ||
return axial_flips |
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.
Should we call these axial_flips
still? It might also make it clearer if it was some kind of mapping
{
"Coronal": -1,
"Sagittal": 1,
"Axial": -1
}
so that its use in _apply_axial_flips_to_annotations
is clearer than axial_flips[index], which is hard to read
if slot_metadata["legacy"]: | ||
continue | ||
|
||
axial_flips = slot_metadata["axial_flips"] |
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.
The logic below
-
Should probably be deduplicated out into helper functions
-
If the changes suggested for
_flip_annotation_in_x_or_y
and_flip_raster_layer_in_x_or_y
are taken, then can remove a lot of the if elses here -
If the changes suggested for the
axial_flips
renaming/changed structure are taken, should reflect here -
Can we also loop over frame_idx, frame_annotation once? aren't the flips order independent?
Problem
When importing non-NifTI annotations, we do not consider if the target dataset items were medical items with flipped X, Y, or Z axes. Non-NifTI annotations generated for items that were re-oriented (All non
LPI
files) out of the platform will appear flipped in at least one axis when importedSolution
This PR:
affine
withoriginal_affine
in the same way that we already compute transforms for NifTI exportsRaster annotations require alternative handling to flip in X & Y. This requires decoding the
dense_rle
, flipping the decoded data, then re-encoding it into anotherdense_rle
We only apply these flips to post-
MED_2D_VIEWER
files. This PR also takes the opportunity to refactor_get_remote_medical_file_transform_requirements
into the simplier_get_remote_file_medical_metadata
which returns a single more understandable dictionary of dataThis cannot be released until DAR-5589 is ready to merge on the backend
Changelog
Applied re-orientation of non-NifTI annotations upon import to medical dataset slots