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

make convert_image_dtype scriptable #2485

Merged
merged 10 commits into from
Oct 5, 2020

Conversation

nairbv
Copy link
Contributor

@nairbv nairbv commented Jul 17, 2020

I'm not sure if this is a good idea since it might be slower, but wanted to see if I could make convert_image_dtype scriptable.

iinfo isn't supported by torchscript: pytorch/pytorch#41492

This PR also moves convert_image_dtype to functional_tensor since it only applies to tensor ops, and because functional_tensor can't import from functional.

@nairbv nairbv force-pushed the scriptable_dtype_conversion branch from c8c30da to 794bbb1 Compare July 17, 2020 18:10
@nairbv nairbv requested a review from fmassa July 17, 2020 18:10
@nairbv nairbv force-pushed the scriptable_dtype_conversion branch from bde5973 to f610463 Compare July 17, 2020 18:42
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.

@nairbv I've already started this in #2313. If we want to do this now, maybe we can "merge" the PRs.

@nairbv
Copy link
Contributor Author

nairbv commented Jul 17, 2020

@nairbv I've already started this in #2313. If we want to do this now, maybe we can "merge" the PRs.

ah, I should have checked first. I was curious if I could and didn't realize someone was working on it already.

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 @nairbv and sorry for the delay in reviewing!

I've left a few comments, let me know what you think

@pmeier
Copy link
Collaborator

pmeier commented Aug 7, 2020

@fmassa Do you want me to close #2313 since it seems we are moving forward with this PR?

@codecov
Copy link

codecov bot commented Aug 7, 2020

Codecov Report

Merging #2485 into master will decrease coverage by 0.11%.
The diff coverage is 93.33%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #2485      +/-   ##
==========================================
- Coverage   73.07%   72.95%   -0.12%     
==========================================
  Files          96       96              
  Lines        8304     8319      +15     
  Branches     1292     1293       +1     
==========================================
+ Hits         6068     6069       +1     
- Misses       1838     1850      +12     
- Partials      398      400       +2     
Impacted Files Coverage Δ
torchvision/transforms/functional.py 80.36% <33.33%> (-1.77%) ⬇️
torchvision/transforms/functional_tensor.py 72.41% <97.43%> (+2.33%) ⬆️
torchvision/transforms/transforms.py 80.92% <100.00%> (+0.03%) ⬆️
torchvision/extension.py 48.64% <0.00%> (-21.63%) ⬇️
torchvision/ops/deform_conv.py 66.66% <0.00%> (-3.34%) ⬇️
torchvision/models/detection/rpn.py 92.73% <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 588f7ae...0790f30. Read the comment docs.

@fmassa
Copy link
Member

fmassa commented Aug 7, 2020

@pmeier yes, let's move forward with this PR, thanks a lot for working on that other PR!

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 changes @nairbv !

Can you add scriptability tests for convert_image_dtype, following the tests from @pmeier in #2313 ?

I've also left a few more comments, after which this PR is good to merge.

@vfdev-5 vfdev-5 requested a review from fmassa October 2, 2020 17:15
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 a lot @nairbv, @pmeier and @vfdev-5 for working on this!

@fmassa fmassa merged commit c542137 into pytorch:master Oct 5, 2020
@nairbv
Copy link
Contributor Author

nairbv commented Oct 5, 2020

Thanks @pmeier and @vfdev-5!

bryant1410 pushed a commit to bryant1410/vision-1 that referenced this pull request Nov 22, 2020
* make convert_image_dtype scriptable

* move convert dtype to functional_tensor since only works on tensors

* retain availability of convert_image_dtype in functional.py

* Update code and tests

* Replaced int by torch.dtype

* int -> torch.dtype and use F instead of F_t

* Update functional_tensor.py

Co-authored-by: vfdev-5 <vfdev.5@gmail.com>
vfdev-5 added a commit to Quansight/vision that referenced this pull request Dec 4, 2020
* make convert_image_dtype scriptable

* move convert dtype to functional_tensor since only works on tensors

* retain availability of convert_image_dtype in functional.py

* Update code and tests

* Replaced int by torch.dtype

* int -> torch.dtype and use F instead of F_t

* Update functional_tensor.py

Co-authored-by: vfdev-5 <vfdev.5@gmail.com>
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.

4 participants