Skip to content

Bugfix - same output for PIL and tensor when centercrop size is greater than imgsize #3333

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

Merged
merged 7 commits into from
Feb 2, 2021
Merged

Bugfix - same output for PIL and tensor when centercrop size is greater than imgsize #3333

merged 7 commits into from
Feb 2, 2021

Conversation

saurabheights
Copy link
Contributor

@saurabheights saurabheights commented Feb 1, 2021

This PR fixes #3297

@datumbox datumbox requested a review from vfdev-5 February 1, 2021 13:56
Copy link
Collaborator

@vfdev-5 vfdev-5 left a comment

Choose a reason for hiding this comment

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

Looks good, thanks @saurabheights !
A nit comment to improve the code a bit

@codecov
Copy link

codecov bot commented Feb 1, 2021

Codecov Report

Merging #3333 (9ccb744) into master (859a535) will decrease coverage by 0.01%.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #3333      +/-   ##
==========================================
- Coverage   73.90%   73.88%   -0.02%     
==========================================
  Files         104      104              
  Lines        9618     9624       +6     
  Branches     1544     1546       +2     
==========================================
+ Hits         7108     7111       +3     
- Misses       2028     2030       +2     
- Partials      482      483       +1     
Impacted Files Coverage Δ
torchvision/transforms/transforms.py 83.68% <ø> (-0.43%) ⬇️
torchvision/transforms/functional.py 81.73% <100.00%> (+0.25%) ⬆️

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 859a535...9ccb744. Read the comment docs.

Copy link
Collaborator

@vfdev-5 vfdev-5 left a comment

Choose a reason for hiding this comment

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

Looks good, thanks @saurabheights !
Just a nit remark about randomness in the tests. I know that it was already there and you replicated that, but from a practical point of view it will be hard to debug random parameters... Anyway, this improvement can be addressed in future.

@saurabheights
Copy link
Contributor Author

@vfdev-5 Just setting even_image_size to (20, 20) should be enough. It still tests all possibilities(even odd img/crop sizes, different size diff). If you want, I can push that change.

Also, regarding drop in coverage, not 100% sure but I think it comes from other tests testing center crop method but not using test condition of cropsize>imgsize. However, adding test conditions to other tests would take me some time.

@vfdev-5
Copy link
Collaborator

vfdev-5 commented Feb 2, 2021

@saurabheights let's ignore codecov failure.

Just setting even_image_size to (20, 20) should be enough. It still tests all possibilities(even odd img/crop sizes, different size diff). If you want, I can push that change.

Yes, I understand. Anyway, let's do it in another PR. My remark about both random params: even_image_size and delta.

@vfdev-5 vfdev-5 merged commit d5096a7 into pytorch:master Feb 2, 2021
facebook-github-bot pushed a commit that referenced this pull request Feb 4, 2021
…er than imgsize (#3333)

Summary:
* Renamed original method to test center crop

* Added test method, docs and added padding when imgsize < cropsize.

* BugFix - keep odd_crop_size odd

* Do not crop when image size after padding matches crop size; updated test.

Reviewed By: datumbox

Differential Revision: D26226610

fbshipit-source-id: d1697edc05f4dfe3469443ca88428a6466cc7eee

Co-authored-by: Vasilis Vryniotis <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Inconsistent API for tensor and PIL image for CenterCrop
4 participants