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 Gamma casting #3472

Merged
merged 6 commits into from
Mar 2, 2021
Merged

fixed Gamma casting #3472

merged 6 commits into from
Mar 2, 2021

Conversation

sanketsans
Copy link
Contributor

Fixes #3067

Removed the double casting of the result to the original dtype.
Now, the casting is only done if the original dtype is different from float32 type which is used to adjust the gamma of the image / tensor.

Since the method requires the image dtype to be in float format. If not, then line no. 400 converts it to the float format. Then needs just one check to change it back to its original format. If it is already in float format then we don't need to change it.

result = img
dtype = img.dtype
if not torch.is_floating_point(img):
result = convert_image_dtype(result, torch.float32)
result = (gain * result ** gamma).clamp(0, 1)
result = convert_image_dtype(result, dtype)
result = result.to(dtype)
return result

Copy link
Contributor

@datumbox datumbox left a comment

Choose a reason for hiding this comment

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

@sanketsans Please avoid opening new PRs in relation to this, so that we can keep track of the discussions in one place (for future reference previously related PRs are #3470, #3471).

As you can see your current PR breaks the unit-tests. This is because the convert_image_dtype() does more than converting the image to specific dtype and thus can't be replaced with a simple to(). As discussed at #3067 (comment) and #3471, the only necessary modification for fixing the issue is just to remove the unnecessary to() call.

If you decide to update this PR with the requested change, please ping me to review it again. Thanks!

Modified the .to() method to convert_image_dtype() method.
@sanketsans
Copy link
Contributor Author

@datumbox Sorry about multiple PRs. I will make sure to avoid it in the future.
I have committed the necessary changes as you mentioned. You can review it.
Thanks

@codecov
Copy link

codecov bot commented Mar 2, 2021

Codecov Report

Merging #3472 (6f5b065) into master (f637c63) will decrease coverage by 0.00%.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #3472      +/-   ##
==========================================
- Coverage   78.70%   78.69%   -0.01%     
==========================================
  Files         105      105              
  Lines        9705     9704       -1     
  Branches     1555     1555              
==========================================
- Hits         7638     7637       -1     
  Misses       1576     1576              
  Partials      491      491              
Impacted Files Coverage Δ
torchvision/transforms/functional_tensor.py 79.19% <ø> (-0.04%) ⬇️

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 f637c63...6f5b065. Read the comment docs.

torchvision/transforms/functional_tensor.py Outdated Show resolved Hide resolved
torchvision/transforms/functional_tensor.py Outdated Show resolved Hide resolved
@datumbox datumbox merged commit 8a82017 into pytorch:master Mar 2, 2021
facebook-github-bot pushed a commit that referenced this pull request Mar 10, 2021
Summary:
* fixed origin head

* fixed inconsistent casting

* updated functional_tensor.py

Modified the .to() method to convert_image_dtype() method.

* Apply suggestions from code review

Reviewed By: NicolasHug, cpuhrsch

Differential Revision: D26945731

fbshipit-source-id: eab2e30c37bc1d29cae2d0921c869af22b49866a

Co-authored-by: Vasilis Vryniotis <datumbox@users.noreply.github.com>
@datumbox datumbox added bug and removed fix labels Apr 27, 2021
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.

Investigate inconsistent casting inside functional_tensor.py
4 participants