Add 'crop' option to RandomRotation#9458
Conversation
🔗 Helpful Links🧪 See artifacts and rendered test results at hud.pytorch.org/pr/pytorch/vision/9458
Note: Links to docs will display an error until the docs builds have been completed. This comment was automatically generated by Dr. CI and updates every 15 minutes. |
sg3-141-592
force-pushed
the
randomrotate-cropping
branch
2 times, most recently
from
db55653 to
87ef6ab
Compare
sg3-141-592
changed the title
There was a problem hiding this comment.
@sg3-141-592 Thanks a lot for this PR! I left some review comments. Feel free to take a look and address them.
By the way, there are no tests for negative angles. Can we add one? Something like asserting _largest_inscribed_crop_size(100, 100, -30) == _largest_inscribed_crop_size(100, 100, 30) to verify the abs()on sin/cos handles negative angles correctly.
Also, please address the test failure for linting. We need to format files with µfmt.
|
|
||
| # Use floor (int()) to guarantee the crop region contains no fill pixels. | ||
| # Clamp to image dimensions for edge cases like wide images rotated near 90°. | ||
| return min(int(crop_h), height), min(int(crop_w), width) |
There was a problem hiding this comment.
Would math.floor() be more appropriate here? int() truncates toward zero rather than rounding down, and math.floor() would make the "round down" intent more explicit. (Both behave the same for positive values, which is always the case here, so this is just a readability suggestion.)
There was a problem hiding this comment.
I agree, math.floor is more clear. Updated ✔️
| assert output.min().item() > 0, "crop=True output should have no fill pixels" | ||
| assert output.shape[-2] < h or output.shape[-1] < w, "crop=True should reduce at least one dimension" |
There was a problem hiding this comment.
We don't need specify anything beyond the assert, pytest is already good at showing the right thing. Please remove the message after assert.
There was a problem hiding this comment.
Sure, removed the superfluous messages
| transforms.RandomRotation(30, expand=True, crop=True) | ||
|
|
||
| @pytest.mark.parametrize("angle", [0.0, 90.0, 180.0, 270.0]) | ||
| def test_transform_crop_zero_angle_preserves_size(self, angle): |
There was a problem hiding this comment.
The test name "test_transform_crop_zero_angle_preserves_size" says "zero_angle" but it tests 0°, 90°, 180°, and 270°, which is a bit misleading. Could you please rename it?
There was a problem hiding this comment.
Sure, updated to test_transform_crop_right_angle_preserves_size
There was a problem hiding this comment.
Now replaced with two test functions because of an issue found below with different canvas sizes for this test case
test_transform_crop_half_turntest_transform_crop_quarter_turn
| @pytest.mark.parametrize("angle", [0.0, 90.0, 180.0, 270.0]) | ||
| def test_transform_crop_zero_angle_preserves_size(self, angle): |
There was a problem hiding this comment.
Can we add non-square sizes by parametrizing over non-square sizes like (120, 80) here?
There was a problem hiding this comment.
Sure, adding this test case found an issue with our early returns for right angle rotations. For 90 deg and 270 deg we need to return the min(w,h) square that sits inside the canvas.
def _largest_inscribed_crop_size(width: int, height: int, angle: float):
...
# Guard against small values of sin_a or cos_a that will cause us to truncate a pixel
# off rotations that introduce no padding (e.g. 90° or 180°).
if sin_a < 1e-10:
return height, width
if cos_a < 1e-10:
- return width, height
+ # 90°/270°: rotated rectangle has dims swapped; clamp to canvas (h, w).
+ return min(width, height), min(height, width)
Note: With this fix I've decided to split test_transform_crop_right_angle_preserves_size into two different cases for the different early exit paths
test_transform_crop_half_turntest_transform_crop_quarter_turn
sg3-141-592
force-pushed
the
randomrotate-cropping
branch
2 times, most recently
from
e837915 to
61dd89f
Compare
…ve any padding regions introduced by the rotation
sg3-141-592
force-pushed
the
randomrotate-cropping
branch
from
61dd89f to
648bfb4
Compare
|
Added test suggested test case for negative angles def test_largest_inscribed_crop_size(self):
...
# Negative angles are symmetric to positive ones (abs() on sin/cos)
for w, h, a in [(100, 100, 30), (200, 100, 15), (120, 80, 45)]:
assert _largest_inscribed_crop_size(w, h, -a) == _largest_inscribed_crop_size(w, h, a) |
|
@sg3-141-592 Thanks a lot for addressing the comments in detail! @NicolasHug Could you please take a look at this PR as well? Please feel free to leave any additional comments. |
2 participants
Add 'crop' option to
RandomRotationthat centre crops the rotated image to remove any padding regions introduced by the rotation #9147cropandexpandare mutually exlusive.This is based off the approach in albumentations - rotate.py. Which in turn is taken from here https://stackoverflow.com/questions/16702966/rotate-image-and-crop-out-black-borders. Slight difference is we use int() (floor) for the final crop dimensions rather than Albumentations' coordinate-based rounding, to guarantee no fill pixels appear in the output.
I've added a
RandomRotation._extract_params_for_v1_transformoverride to strip the crop parameter fortorch.jit.script()compatibility, and to raise aValueErrorifcrop=Truesince the v1 transform has no equivalent.