Skip to content
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

Unified input for resized crop op #2396

Merged
merged 7 commits into from
Jul 7, 2020

Conversation

vfdev-5
Copy link
Collaborator

@vfdev-5 vfdev-5 commented Jul 6, 2020

Related to #2292

Description:

Blocked by #2394

@vfdev-5 vfdev-5 marked this pull request as ready for review July 7, 2020 09:23
top (int): Vertical component of the top left corner of the crop box.
left (int): Horizontal component of the top left corner of the crop box.
height (int): Height of the crop box.
width (int): Width of the crop box.
size (sequence or int): Desired output size. Same semantics as ``resize``.
interpolation (int, optional): Desired interpolation. Default is
``PIL.Image.BILINEAR``.
interpolation (int, optional): Desired interpolation. Default is ``PIL.Image.BILINEAR``.
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Technically, this is not "optional" (Optional is for None).

Copy link
Member

Choose a reason for hiding this comment

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

yeah, this is a potential issue with the documentation: it has a default value, so the user doesn't need to specify it, so it is in some sense optional, but I see your point.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Seems like it is not the only usage like that. Maybe, we can leave it for now ?

Copy link
Member

Choose a reason for hiding this comment

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

yeah, sounds good to me

@codecov
Copy link

codecov bot commented Jul 7, 2020

Codecov Report

Merging #2396 into master will increase coverage by 0.02%.
The diff coverage is 64.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #2396      +/-   ##
==========================================
+ Coverage   70.65%   70.68%   +0.02%     
==========================================
  Files          94       94              
  Lines        7897     7905       +8     
  Branches     1241     1245       +4     
==========================================
+ Hits         5580     5588       +8     
+ Misses       1934     1932       -2     
- Partials      383      385       +2     
Impacted Files Coverage Δ
torchvision/transforms/transforms.py 76.04% <60.86%> (+0.02%) ⬆️
torchvision/transforms/functional.py 80.92% <100.00%> (+1.09%) ⬆️
torchvision/transforms/functional_tensor.py 65.41% <100.00%> (+0.83%) ⬆️
torchvision/transforms/functional_pil.py 61.53% <0.00%> (-2.80%) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update e212cc8...d7ed08f. Read the comment docs.

Copy link
Member

@fmassa fmassa left a comment

Choose a reason for hiding this comment

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

Thanks for the PR, looks good to me!

There are just one question that I think could deserve a comment in the code, otherwise looks good to merge!

@@ -532,7 +532,7 @@ def resize(img: Tensor, size: List[int], interpolation: int = 2) -> Tensor:
elif len(size) < 2:
size_w, size_h = size[0], size[0]
else:
size_w, size_h = size[0], size[1]
size_w, size_h = size[1], size[0] # Convention (h, w)
Copy link
Member

Choose a reason for hiding this comment

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

Thanks for the fix!

if 0 < w <= width and 0 < h <= height:
i = random.randint(0, height - h)
j = random.randint(0, width - w)
if 0 < w < width and 0 < h < height:
Copy link
Member

Choose a reason for hiding this comment

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

was the <= a bug before? Or is ti because random.randint is inclusive on end value and torch.randint is exclusive on end value?

i = torch.randint(0, height - h, size=(1,)).item()
j = torch.randint(0, width - w, size=(1,)).item()
if 0 < w <= width and 0 < h <= height:
i = torch.randint(0, max(height - h, 1), size=(1,)).item()
Copy link
Member

Choose a reason for hiding this comment

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

don't we need to add a +1 to account for the previous behavior, instead of making it max(..., 1)?

@fmassa fmassa merged commit 9b80465 into pytorch:master Jul 7, 2020
@vfdev-5 vfdev-5 deleted the vfdev-5/issue-2292-resized-crop branch July 7, 2020 14:37
@vfdev-5 vfdev-5 mentioned this pull request Jul 7, 2020
16 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants