Skip to content

allow slicing with SpatialCrop(d)#1954

Merged
wyli merged 10 commits intoProject-MONAI:masterfrom
rijobro:spatialcrop_slices
Apr 16, 2021
Merged

allow slicing with SpatialCrop(d)#1954
wyli merged 10 commits intoProject-MONAI:masterfrom
rijobro:spatialcrop_slices

Conversation

@rijobro
Copy link
Copy Markdown
Contributor

@rijobro rijobro commented Apr 6, 2021 •
edited
Loading

Allow the use of a list of slice when cropping. This allows indexing from the back or None to not crop in that direction of that dimension. As requested here: #1929 and #1935.

I decided to add slices rather than modifying roi_start to accept -ve numbers, for example, as this would break backwards compatibility (and doesn't allow for None).

Description

Transform can now be called with e.g., SpatialCrop(roi_slices=[slice(-10, None), slice(None)]) to take the last 10 elements in the first dimension, and leave the second dimension untouched.

Implement the inverse using the slices rather than roi_start and roi_end, as was done previously.

PR is backwards compatible.

Status

Ready

Types of changes

  • Non-breaking change (fix or new feature that would not break existing functionality).
  • New tests added to cover the changes.
  • Integration tests passed locally by running ./runtests.sh -f -u --net --coverage.
  • Quick tests passed locally by running ./runtests.sh --quick --unittests.
  • In-line docstrings updated.
  • Documentation updated, tested make html command in the docs/ folder.

@wyli
Copy link
Copy Markdown
Contributor

wyli commented Apr 12, 2021

could this be used to support #1993?

@rijobro
Copy link
Copy Markdown
Contributor Author

rijobro commented Apr 14, 2021

@wyli do you know if the line I deleted in the most recent commit was required? box_start and box_end are both defined a few lines earlier.

@wyli
Copy link
Copy Markdown
Contributor

wyli commented Apr 14, 2021

@wyli do you know if the line I deleted in the most recent commit was required? box_start and box_end are both defined a few lines earlier.

I think those are needed because SpatialCrop will refine those initial values, perhaps we can still keep self.roi_start and self.roi_end in SpatialCrop?

@wyli
Copy link
Copy Markdown
Contributor

wyli commented Apr 14, 2021

similarly the current inverse logic uses the original box_start/end, we might have some issues...

MONAI/monai/transforms/croppad/dictionary.py

Lines 602 to 604 in 5f47407

cropper = SpatialCrop(roi_start=box_start, roi_end=box_end)
for key in self.key_iterator(d):
self.push_transform(d, key, extra_info={"box_start": box_start, "box_end": box_end})

@rijobro
Copy link
Copy Markdown
Contributor Author

rijobro commented Apr 15, 2021 •
edited
Loading

The inverse of CropForegroundd is fine since the limits get calculated by generate_spatial_bounding_box. I've updated the deepgrow transform to get the start and end bounding box from the .start and .stop of slice.

If possible, I'd rather not keep self.roi_start and self.roi_end. It would make life easier in the short-run, but given that the bounding box is more general than slice, keeping both would require a lot if if/else statements.

wyli
wyli approved these changes Apr 16, 2021
View reviewed changes
Copy link
Copy Markdown
Contributor

@wyli wyli left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

thanks!

@wyli wyli enabled auto-merge (squash) April 16, 2021 11:20
@wyli wyli merged commit 9f4da6a into Project-MONAI:master Apr 16, 2021
@rijobro rijobro deleted the spatialcrop_slices branch April 26, 2021 09:36
@Nic-Ma Nic-Ma mentioned this pull request Apr 26, 2021
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@wyli wyli wyli approved these changes

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@rijobro @wyli