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

Add float support to ColorJitter #2672

Open
wants to merge 2 commits into
base: main
Choose a base branch
from
Open

Add float support to ColorJitter #2672

wants to merge 2 commits into from

Conversation

vballoli
Copy link
Contributor

Fixes #2669 and adds relevant typing to the arguments.

@codecov
Copy link

codecov bot commented Sep 13, 2020

Codecov Report

Merging #2672 into master will decrease coverage by 0.06%.
The diff coverage is 33.33%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #2672      +/-   ##
==========================================
- Coverage   72.41%   72.35%   -0.07%     
==========================================
  Files          95       95              
  Lines        8245     8254       +9     
  Branches     1308     1312       +4     
==========================================
+ Hits         5971     5972       +1     
- Misses       1858     1866       +8     
  Partials      416      416              
Impacted Files Coverage Δ
torchvision/transforms/transforms.py 75.59% <33.33%> (-0.94%) ⬇️

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 a4736ea...2b0c8a0. Read the comment docs.

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 for the PR!

I think there is an issue with the current way you implement this.

Also, could you add tests for using the get_params interface for the case you are trying to fix?

@@ -1052,18 +1054,26 @@ def get_params(brightness, contrast, saturation, hue):
transforms = []

if brightness is not None:
if isinstance(brightness, float):
ColorJitter._check_input(brightness, 'brightness')
Copy link
Member

Choose a reason for hiding this comment

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

We are using the returned value of _check_input, so this doesn't actually do anything IMO.

Could we instead just do

brightness = ColorJitter._check_input(brightness, 'brightness')

without the conditional on float?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah that definitely makes sense, not sure how I missed that.

@vfdev-5
Copy link
Collaborator

vfdev-5 commented Sep 14, 2020

@fmassa according to what we discussed previously, maybe it would be better to get back ColorJitter.get_params to forward ? In this case, fix with Union wont work with torch jit script, right ?

@fmassa
Copy link
Member

fmassa commented Sep 14, 2020

@vfdev-5 the Union is not the only issue, but also that get_params returns a Transforms, which IMO doesn't work with torchscript

@vfdev-5
Copy link
Collaborator

vfdev-5 commented Sep 14, 2020

@fmassa that's true about current Compose implementation but taking into account #2645 it may be possible to reuse ColorJitter.get_params later ?

@fmassa
Copy link
Member

fmassa commented Sep 22, 2020

@vfdev-5

@fmassa that's true about current Compose implementation but taking into account #2645 it may be possible to reuse ColorJitter.get_params later ?

Maybe in the future we could change that, but I wouldn't focus on it now. But we could have an open issue to track this inconsistency down.

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.

transforms.ColorJitter().get_params(...) does not support float inputs
4 participants