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

Fixed resize to only match the smaller edge when an int is given #2518

Merged
merged 2 commits into from
Aug 3, 2020

Conversation

ag14774
Copy link
Contributor

@ag14774 ag14774 commented Jul 29, 2020

The smaller edge should be matched and the aspect ratio should be kept the same only when one int or a list of one element is given. When an explicit tuple of (h,w) is given, the image should always be resized to the requested size

This fixes #2517

@codecov
Copy link

codecov bot commented Jul 29, 2020

Codecov Report

Merging #2518 into master will increase coverage by 0.02%.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #2518      +/-   ##
==========================================
+ Coverage   70.65%   70.67%   +0.02%     
==========================================
  Files          94       94              
  Lines        8007     8007              
  Branches     1278     1278              
==========================================
+ Hits         5657     5659       +2     
+ Misses       1945     1944       -1     
+ Partials      405      404       -1     
Impacted Files Coverage Δ
torchvision/transforms/functional_tensor.py 64.91% <100.00%> (ø)
torchvision/transforms/functional_pil.py 64.44% <0.00%> (+1.11%) ⬆️

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 300ef76...aaa6f68. Read the comment docs.

Copy link
Collaborator

@pmeier pmeier left a comment

Choose a reason for hiding this comment

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

Hi @ag14774 and thanks for the PR! Could you add a test or give an example which highlights a scenario where the original code fails?

@ag14774
Copy link
Contributor Author

ag14774 commented Jul 30, 2020

Hi @pmeier! Thanks for the review. I added a test that I believe will lead to different results between the PIL resize and the tensor resize (with the old code) but I have not run the tests locally to confirm.

Comment on lines -589 to -590
if (w <= h and w == size_w) or (h <= w and h == size_h):
return img
Copy link
Collaborator

Choose a reason for hiding this comment

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

@vfdev-5 Could you explain what your intention for this guarding clause was? Is this just to detect the case if no resizing needs to happen? If yes, why not simply do

if (w,h) == (size_w, size_h):
    return 

Copy link
Contributor Author

@ag14774 ag14774 Jul 30, 2020

Choose a reason for hiding this comment

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

I think the intention is that (according to the function docstring), if only one number is given as size, we resize to match the smaller dimension to that number. The check there is for the case when the smaller dimension matches the number given and no resizing needs to happen. But it doesn't mean that both dims need to match

Copy link
Collaborator

Choose a reason for hiding this comment

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

Sorry for delayed reply! Yes, this function should mimic PIL implementation : https://github.com/pytorch/vision/blob/master/torchvision/transforms/functional_pil.py#L324 where definitely this condition is applied in case when if isinstance(size, int) or len(size) == 1. Thanks for the fix @ag14774 !

@fmassa fmassa merged commit df6a796 into pytorch:master Aug 3, 2020
bryant1410 pushed a commit to bryant1410/vision-1 that referenced this pull request Nov 22, 2020
…orch#2518)

* Fixed resize to only match the smaller edge when an int is given instead of always

* Added test cases for resize()
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.

Resize sometimes fails
4 participants