-
Notifications
You must be signed in to change notification settings - Fork 6
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
Crop to physical space #6
Conversation
Before review, can you explain the failing tests? |
Those failed tests are from the second-to-last commit I think. There I made a mistake in the zyx/xyz order convention, which I fixed later. The PR has all tests passing. |
src/picai_prep/preprocessing.py
Outdated
""" | ||
# input conversion and verification | ||
if physical_size is not None: | ||
# convert physical size to voxel size (only supported for SimpleITK) | ||
assert isinstance(image, sitk.Image), "Crop/padding by physical size is only supported for SimpleITK images." |
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.
assert should not be used in production code. use if/raise instead.
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 agree, made a new commit to tackle this. Also, added a new function crop
where a maximum size can be specified. I need this functionality and think others may benefit from it as well, do you think this is the right place to store this function? (as it is not used inside the picai_prep module itself, but does pertain to preprocessing).
- Shared verification of crop/pad plan - Raise error instead of assert - Separate crop function that won't pad (i.e., size is a max size)
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 does have a smell of feature creep. suggest to instead create a setting
crop_only
, making physical_size and/or matrix_size act as the current max_matrix_size, ... instead
src/picai_prep/preprocessing.py
Outdated
@@ -305,15 +317,33 @@ def resample_spacing(self, spacing: Optional[Iterable[float]] = None): | |||
if self.lbl is not None: | |||
self.lbl = resample_img(self.lbl, out_spacing=spacing, is_label=True) | |||
|
|||
def centre_crop_or_pad(self): | |||
"""Centre crop adn/or pad scans and label""" |
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.
typo
src/picai_prep/preprocessing.py
Outdated
# perform centre crop and/or pad | ||
self.centre_crop_or_pad() | ||
|
||
if self.settings.max_matrix_size is not None or self.settings.max_physical_size is not None: |
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.
while I know in the post_init neither matrix or max_matrix can be both set, I still think its safer to change this to elif
Thanks for checking out the changes! I agree fully, implementing the |
src/picai_prep/preprocessing.py
Outdated
|
||
if reference_scan_preprocessed is not None: | ||
image.CopyInformation(reference_scan_preprocessed) | ||
|
||
if interpolator is None: | ||
# determine interpolation method based on image dtype | ||
if "integer" in image.GetPixelIDTypeAsString(): |
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.
add
pixel_id_type = image.GetPixelIDTypeAsString()
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.
Done
Allow crop/pad of scans based on physical size