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 resize op #2394

Merged
merged 4 commits into from
Jul 6, 2020

Conversation

vfdev-5
Copy link
Collaborator

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

Related to #2292

Description:

  • Unified resize operation: T.Resize, F.resize and F_t.resize
  • Added tests

Remark:

  • Only certain PIL interpolations can be supported
  • Tests of pixel values for resize between PIL and Tensors are done for interpolation > nearest as MAE.

@vfdev-5 vfdev-5 mentioned this pull request Jul 6, 2020
16 tasks
@codecov
Copy link

codecov bot commented Jul 6, 2020

Codecov Report

Merging #2394 into master will decrease coverage by 0.15%.
The diff coverage is 66.66%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #2394      +/-   ##
==========================================
- Coverage   70.69%   70.53%   -0.16%     
==========================================
  Files          94       94              
  Lines        7839     7895      +56     
  Branches     1221     1241      +20     
==========================================
+ Hits         5542     5569      +27     
- Misses       1925     1943      +18     
- Partials      372      383      +11     
Impacted Files Coverage Δ
torchvision/transforms/transforms.py 76.02% <50.00%> (-0.53%) ⬇️
torchvision/transforms/functional_tensor.py 64.58% <64.44%> (-0.04%) ⬇️
torchvision/transforms/functional_pil.py 64.33% <71.42%> (+0.92%) ⬆️
torchvision/transforms/functional.py 79.82% <100.00%> (+0.93%) ⬆️
torchvision/extension.py 48.64% <0.00%> (-21.63%) ⬇️
torchvision/models/detection/rpn.py 92.70% <0.00%> (-0.43%) ⬇️

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 75f5b57...8f48a02. Read the comment docs.

@vfdev-5 vfdev-5 force-pushed the vfdev-5/issue-2292-resize branch from d69e4b1 to 965ad6b Compare July 6, 2020 09:31
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 a lot for the PR!

I have a couple of minor comments, otherwise the PR looks good to merge

@@ -1,3 +1,5 @@
from PIL.Image import NEAREST, BILINEAR, BICUBIC
Copy link
Member

Choose a reason for hiding this comment

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

nit: this doesn't seem to be used?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Correct. Will remove it

"{} element tuple/list".format(len(size)))

if interpolation not in [0, 1, 2, 3, 4]:
raise ValueError("Interpolation mode should be either constant, edge, reflect or symmetric")
Copy link
Member

Choose a reason for hiding this comment

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

error message seems off?

size = list(size)

if isinstance(size, list) and len(size) not in [1, 2]:
raise ValueError("Padding must be an int or a 1 or 2 element tuple/list, not a " +
Copy link
Member

Choose a reason for hiding this comment

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

error message seems off?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Thanks !

Comment on lines +315 to +316
# E.g. resized_tensor = [[a, a, b, c, d, d, e, ...]]
# E.g. resized_pil_img = [[a, b, c, c, d, e, f, ...]]
Copy link
Member

Choose a reason for hiding this comment

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

It's a pity that the behavior of nearest interpolate is different between implementations, I would say it could be worth opening an issue in PyTorch to mention this. I also believe that PIL and OpenCV are consistent, which would make for a case to maybe change the implementation in PyTorch to make this more consistent.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Let me check that between PIL and OpenCV and then we decide about PyTorch.

raise ValueError("Padding must be an int or a 1 or 2 element tuple/list, not a " +
"{} element tuple/list".format(len(size)))

if interpolation not in [0, 1, 2, 3, 4]:
Copy link
Member

Choose a reason for hiding this comment

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

Looks like we don't support mode 1 in the _interpolation_modes?

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 a lot!

Let's merge this now, and then discuss if we should open an issue in PyTorch to propose to change the behavior of nearest interpolate.


if need_cast:
if mode == "bicubic":
img = img.clamp(min=0, max=255)
Copy link
Member

Choose a reason for hiding this comment

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

nit: for the future, we might want to change the 255 value to be dtype-dependent. But this can be done in a future, maybe using convert_image_dtype before and after interpolate is called.

@fmassa fmassa merged commit e212cc8 into pytorch:master Jul 6, 2020
@vfdev-5 vfdev-5 deleted the vfdev-5/issue-2292-resize branch July 6, 2020 13:14
de-vri-es pushed a commit to fizyr-forks/torchvision that referenced this pull request Aug 4, 2020
* [WIP] F.resize with tensor

* Adapted T.Resize and F.resize with a test

* According to the review, fixed copy-pasted messages and unused imports
@vfdev-5 vfdev-5 mentioned this pull request Oct 22, 2020
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