Repository navigation
Conversation
Crop.apply_transform and _crop_image_lazy built slice stops as `si - i1 or None`. `si - i1` is already `si` (equivalent to an open-ended stop) when i1 == 0, so the `or None` was never needed for that case -- but it also converts a stop of exactly 0 (cropping an amount equal to the axis's full size, i.e. i1 == si) into None, which means "slice to the end" instead of the intended empty slice. The whole axis was silently left completely uncropped instead of becoming length 0, with no error or warning. Fixed by removing the unneeded `or None`: `si - i1` is correct as a literal stop value in every case (open-ended crop, partial crop, and full-axis crop alike). Added a test for both the eager (Crop.apply_transform) and lazy (_crop_image_lazy) paths. Mutation-verified: reverting to `or None` in either file fails exactly the new tests for that file.
Contributor
There was a problem hiding this comment.
Copilot was unable to review this pull request because the user who requested the review has reached their quota limit.
An organization owner can choose the repository organization as the billing source for reviews requested by members with a paid Copilot license. Learn about Copilot code review billing.
Author
|
Checking in on this one — still green and mergeable, no review yet. Happy to make any further changes if needed. |
This branch has not been deployed
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Bug
Crop.apply_transform(src/torchio/transforms/spatial/crop.py) and_crop_image_lazy(src/torchio/transforms/spatial/crop_or_pad.py) both build slice stops as:si - i1already equalssi(an open-ended stop, same asNone) wheni1 == 0. Theor Nonedoesn't help that case — but it also converts a stop of exactly0intoNone. A stop of0happens wheni1 == si, i.e. the user crops an amount equal to the axis's full size (intending to remove the whole axis).slice(i0, None)means "to the end," the opposite of the intended empty slice — the axis is silently left completely uncropped instead of becoming length 0. No error, no warning.Reproduction:
Fix
Remove the unneeded
or None:si - i1is correct as a literal stop value in every case (open-ended, partial, and full-axis crop alike), since slicing tosion an axis of lengthsiis identical to slicing toNone.Tests
Added one test per affected path:
test_crop_entire_axis_from_the_end/test_crop_entire_axis_from_the_end_all_axes(tests/test_crop.py) for the eagerCrop.apply_transformpath.test_crop_image_lazy_entire_axis_from_the_end(tests/test_crop_or_pad.py) for the lazy_crop_image_lazypath.Mutation-verified: reverting either file's fix back to
... or Nonefails exactly the new test(s) for that file, with the rest oftests/test_crop.pyandtests/test_crop_or_pad.py(84 tests total) still green. Fulltests/test_crop.py+tests/test_crop_or_pad.pysuite: 84 passed. Broader-k "crop or pad or spatial"slice: 252 passed.