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

ColorJitter Enhancement #548

Merged
merged 5 commits into from
Jul 30, 2018
Merged

ColorJitter Enhancement #548

merged 5 commits into from
Jul 30, 2018

Conversation

yaox12
Copy link
Contributor

@yaox12 yaox12 commented Jul 16, 2018

Set custom min and max bounds by inputing a tuple for each parameter in ColorJitter.

See issue #545

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've left a few comments that I think could improve the functionality and make it less error-prone if in the future we want to extend its functionality.

@@ -754,20 +758,53 @@ def get_params(brightness, contrast, saturation, hue):
saturation in a random order.
"""
transforms = []
if brightness > 0:
brightness_factor = None
if isinstance(brightness, float) and brightness >= 0:

This comment was marked as off-topic.

brightness_factor = random.uniform(max(0, 1 - brightness), 1 + brightness)
elif isinstance(brightness, tuple) and len(brightness) == 2:

This comment was marked as off-topic.

@yaox12
Copy link
Contributor Author

yaox12 commented Jul 16, 2018

I've improved it according to your advice. Please review it. @fmassa

@vfdev-5
Copy link
Collaborator

vfdev-5 commented Jul 16, 2018

@yaox12 just wonder in which case the following conditions are met like

if brightness_factor is not None:

in _sample_from I think there is no possibility to return None...

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.

I made another round of comments. Let me know what you think

@@ -753,21 +757,39 @@ def get_params(brightness, contrast, saturation, hue):
Transform which randomly adjusts brightness, contrast and
saturation in a random order.
"""
def _sample_from(value, name):
factor = None

This comment was marked as off-topic.

factor = None
if isinstance(value, numbers.Number) and value >= 0:
if name == 'hue':
factor = random.uniform(-value, value)

This comment was marked as off-topic.

This comment was marked as off-topic.

This comment was marked as off-topic.

This comment was marked as off-topic.

This comment was marked as off-topic.

This comment was marked as off-topic.

brightness_factor = random.uniform(max(0, 1 - brightness), 1 + brightness)

brightness_factor = _sample_from(brightness, 'brightness')
if brightness_factor is not None:
transforms.append(Lambda(lambda img: F.adjust_brightness(img, brightness_factor)))

This comment was marked as off-topic.

This comment was marked as off-topic.

This comment was marked as off-topic.

@yaox12
Copy link
Contributor Author

yaox12 commented Jul 16, 2018

In the previous code, if brightness = 0, then nothing would happen. In my code, it will call F.adjust_brightness(img, 1), though it will return img without any change.
I would like to make this case return None, but I find it will bring more if. Do you have any elegant solution? @vfdev-5

@vfdev-5
Copy link
Collaborator

vfdev-5 commented Jul 16, 2018

@yaox12 Maybe something like in the previous code could help. Previously, if brightness/saturation/... value is 0 than ignore it, we can replace it with None and check as previously :

if saturation is not None:
    saturation_factor = _sample_from(saturation, 'saturation')
    transforms.append(Lambda(lambda img: F.adjust_saturation(img, saturation_factor)))

or if we want to keep it as before

if saturation > 0:
    saturation_factor = _sample_from(saturation, 'saturation')
    transforms.append(Lambda(lambda img: F.adjust_saturation(img, saturation_factor)))

HTH

@yaox12
Copy link
Contributor Author

yaox12 commented Jul 17, 2018

Check input in __init__. Welcome for comments.

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.

LGTM, thanks!

@fmassa fmassa merged commit b51a2c3 into pytorch:master Jul 30, 2018
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.

3 participants