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.

Sign up for GitHub

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

Jump to bottom

[BC-breaking] Unified input for F.perspective #2558

Merged
merged 5 commits into from
Aug 7, 2020
Merged

[BC-breaking] Unified input for F.perspective #2558

merged 5 commits into from
Aug 7, 2020

Conversation

vfdev-5
Copy link
Collaborator

@vfdev-5 vfdev-5 commented Aug 6, 2020 •
edited by fmassa
Loading

Related to #2292

Description:

  • Unified input for F.perspective
  • added tests
  • updated docs

BC-breaking change because we changed the default value of interpolate to be BILINEAR instead of BICUBIC for perspective

@vfdev-5 vfdev-5 force-pushed the vfdev-5/issue-2292-perspective branch from 983aaca to 205051e Compare August 6, 2020 20:32
@codecov
Copy link

codecov bot commented Aug 6, 2020 •
edited
Loading

Codecov Report

Merging #2558 into master will increase coverage by 0.25%.
The diff coverage is 80.43%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #2558      +/-   ##
==========================================
+ Coverage   71.81%   72.07%   +0.25%     
==========================================
  Files          94       94              
  Lines        8080     8244     +164     
  Branches     1282     1353      +71     
==========================================
+ Hits         5803     5942     +139     
- Misses       1868     1872       +4     
- Partials      409      430      +21
Impacted Files Coverage Δ
torchvision/transforms/functional_pil.py 65.07% <66.66%> (+0.05%) ⬆️
torchvision/transforms/functional_tensor.py 68.51% <73.07%> (+1.01%) ⬆️
torchvision/transforms/functional.py 80.73% <100.00%> (+0.62%) ⬆️
torchvision/transforms/transforms.py 77.52% <0.00%> (+1.33%) ⬆️

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 025b71d...b758bc3. Read the comment docs.

@vfdev-5 vfdev-5 requested a review from fmassa August 7, 2020 10:20
@vfdev-5 vfdev-5 mentioned this pull request Aug 7, 2020
Closed
16 tasks
fmassa
fmassa requested changes Aug 7, 2020
View reviewed changes
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 @vfdev-5 , looks great!

I think we should do something wrt the silent change of behavior in the interpolation mode, otherwise it can be very confusing for users.

torchvision/transforms/functional.py Outdated
Comment on lines 520 to 521
# RuntimeError: Expected type hint for result of tolist()
return [float(i.item()) for i in res[:, 0]]
Copy link
Member

Choose a reason for hiding this comment

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

I believe you can do something like

output : List[float] = res.squeeze(1).tolist()
return output

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Sounds good, especially that we have min pyhon 3.6

torchvision/transforms/functional.py Outdated
Comment on lines 560 to 561
# set to bilinear interpolation
interpolation = 2
Copy link
Member

Choose a reason for hiding this comment

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

I'm not sure that silently changing the value requested by the user is a good thing to do here.

It is unfortunate that bicubic is not supported in PyTorch yet, but I think we should try to be consistent here (the user requested bicubic and we silently compute bilinear).

What about one of the following:

  1. change the default value for the whole function to be BILINEAR, and raise an error if BICUBIC is passed by the user, and add a [BC-breaking] in the tile of the PR
  2. raise a warning before switching the default value here to bilinear

I think I would prefer option 1 here, as option 2 would by default raise warnings to the user.

Copy link
Collaborator Author

@vfdev-5 vfdev-5 Aug 7, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

I agree, that silent changing the value is not good at all ! It was a full-moon or something :) Sorry about that !
I'll remove this part of code. Error is raised inside F_t.affine if resample is not support:
https://github.com/pytorch/vision/pull/2558/files#diff-77c635250f0215f979b2650c383ecbecL637

@vfdev-5 vfdev-5 changed the title Unified input for F.perspective [BC-breaking] Unified input for F.perspective Aug 7, 2020
fmassa
fmassa approved these changes Aug 7, 2020
View reviewed changes
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.

Looks great, thanks!

@fmassa fmassa merged commit 8c7e7bb into pytorch:master Aug 7, 2020
bryant1410 pushed a commit to bryant1410/vision-1 that referenced this pull request Nov 22, 2020
@vfdev-5 @bryant1410
[BC-breaking] Unified input for F.perspective (pytorch#2558)
cd145b1 
* [WIP] Added unified input perspective transformation code

* Unified input for F.perspective
- added tests
- updated docs

* Added more random test configs

* Fixed the code according to PR's review
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@fmassa fmassa fmassa approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@vfdev-5 @fmassa