-
Notifications
You must be signed in to change notification settings - Fork 41
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
[MED-23][external] Support for export from Raster Layer to NifTI #750
[MED-23][external] Support for export from Raster Layer to NifTI #750
Conversation
darwin/exporter/formats/nifti.py
Outdated
frames[frame_idx].annotation_class.name | ||
im_mask = convert_polygons_to_mask(polygons, height=height, width=width) | ||
volume = output_volumes[series_instance_uid] | ||
if view_idx == 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.
is there a reason you use '0' here but XYPLANE above? Are these totally different or is the magic number here meant to be XYPLANE? Same for below
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.
Implemented in the new commit
darwin/exporter/formats/nifti.py
Outdated
frames = annotation.frames | ||
|
||
# define the different planes | ||
XYPLANE = 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.
Are these intrinsinc to nifti and potentially used by other functions? If so can we make this a type
Something like
class Plane(Enum):
XY = 0
XZ = 1
YZ = 2
if view_idx == Plane.XY.value
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.
Implemented in the new commit
darwin/exporter/formats/nifti.py
Outdated
im_mask = convert_polygons_to_mask(polygons, height=height, width=width) | ||
volume = output_volumes[series_instance_uid] | ||
if view_idx == 0: | ||
volume[annotation.annotation_class.name].pixel_array[ |
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 would be great to generalize this function, if only so that it's named and has a docstring. I can't actually tell by inspection what it's meant to be doing too easily
def transform_plane(plane, volume, ...): #or some other more relevant name
""" docstrings explaining high level so that numpy logic is easier to reason about """
if plane == XYZ:
transform_indexes = [np.s_[:], frame_idx, np.s[:]]
...
volume[annotation.annotation_class.name].pixel_array[transform_indexes] = np.logical_or(...)
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.
and I know this is probably some nifti format specific thing but jumping into the code for these specific data formats from the outside is a lot easier if these sorts of transforms are easier to reason about, with names and descriptions
b6b187f
to
eb71db3
Compare
11971af
to
0ec1426
Compare
5d7b086
to
0e18043
Compare
0e18043
to
68ff24b
Compare
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.
LGTM - Tested this commit in a deploybot to ensure we can export multiple mask & polygon classes as NifTI volumes
Problem
Users of Darwin can currently annotate as masks but not export these results to NifTI
Solution
We create a new code path within the same NifTI export logic. This means that for darwin JSON that contains polygons we will still parse these as before and output a NifTI volume. If the user has K mask classes they will receive K output files called
SeriesInstanceUID_classname_m.nii.gz
(NOTE: for nifti uploads SeriesInstanceUID is the filename)Changelog
Introduced initial support for NifTI export from Masks