Skip to content

modified transforms.py to accept list of PIL images #611

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

Closed
wants to merge 10 commits into from
Closed

modified transforms.py to accept list of PIL images #611

wants to merge 10 commits into from

Conversation

quantummole
Copy link

if input is a list of images then output is a list of images else old behaviour is retained
if params need to be computed for every img, then they are computed based on the fist img of the list
this was done to apply the same random transform on each img for example - image segmentation

if input is a list of PIL images it returns a list of transformed imgs else it retains its old behaviour.

if params need to be computed for every img then the params are computed based on the first img of the list.

this change was made to ensure that a set of img have the same random transforms applied to them, for example in the image segmentation.
modified code to accept list of pil images
@fmassa
Copy link
Member

fmassa commented Sep 24, 2018

Hi,

Thanks for the PR!

I'm not sure that this is the way we would want those problems to be handled. It indeed adds a strong assumption that everything in the list if a PIL Image.
This is the case for Image Segmentation, but still breaks some assumptions even in this case.
For example, we don't want to apply bilinear interpolation for the ground-truth segmentation masks, so this should be handled differently and that's not covered in this PR. Same thing for rotations, scalings, etc.

For this reason, I think that we really want to keep the class interface as simple as possible, and let the user leverage the functional interface to perform their fine-grained transformations themselves.

What do you think?

@quantummole
Copy link
Author

You do have a point. Then is it possible for us to have a uniform interface like get_params for transforms like random_grayscale etc which would return a random bool that we can use to chain methods inside the dataset.

@fmassa
Copy link
Member

fmassa commented Sep 24, 2018

Oh, yes, definitely!
RandomGrayscale should have a get_params in there, it was an oversight that it didn't have it.
Ideally, all transforms that have random parameters should have them via a get_params staticmethod.

Could you send a PR fixing this in RandomGrayscale?

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 updating the PR. I have a few comments

@@ -503,6 +515,16 @@ class RandomVerticalFlip(object):
def __init__(self, p=0.5):
self.p = p

def get_params(p):

This comment was marked as off-topic.

@@ -485,7 +496,8 @@ def __call__(self, img):
Returns:
PIL Image: Randomly flipped image.
"""
if random.random() < self.p:
to_flip = self.get_params(self.p)
if to_flip :

This comment was marked as off-topic.

@@ -511,7 +533,8 @@ def __call__(self, img):
Returns:
PIL Image: Randomly flipped image.
"""
if random.random() < self.p:
to_flip = self.get_params(self.p)
if to_flip :

This comment was marked as off-topic.

@@ -1037,6 +1060,7 @@ class Grayscale(object):
def __init__(self, num_output_channels=1):
self.num_output_channels = num_output_channels


This comment was marked as off-topic.

@@ -1068,6 +1092,16 @@ class RandomGrayscale(object):
def __init__(self, p=0.1):
self.p = p

def get_params(p):

This comment was marked as off-topic.

@@ -1077,7 +1111,8 @@ def __call__(self, img):
PIL Image: Randomly grayscaled image.
"""
num_output_channels = 1 if img.mode == 'L' else 3
if random.random() < self.p:
to_convert = self.get_params(self.p)
if to_convert :

This comment was marked as off-topic.

@fmassa
Copy link
Member

fmassa commented Sep 24, 2018

Actually, now I remember why we didn't have get_params for those functions: it was because they were fairly trivial, and generally simpler to implement in the code itself.
I'm not against adding it here, but I'd like to know how you are planning to write your code to support the semantic segmentation use-case

@quantummole
Copy link
Author

I was planning to take a list of transforms as input and then apply them sequentially using compose. I can just pass two lists one for the functional equivalents and one for the actual transforms, now if the actual transforms have a standard interface through which we can get parameters it will make it simpler to abstract away the kind of transforms that we would be applying out of the dataset code

@fmassa
Copy link
Member

fmassa commented Sep 24, 2018

The problem is that get_params is a staticmethod, so it's not that easy to abstract that away.
In those cases, I think it's just way simpler to write your own transform, using purely the functional interface

class MyTransform(object):
    def __init__(self, **kwargs):
        # store params here

    def __call__(self, img, masks, etc):
        if random.rand() > self.prob1:
               # do my magic
        return img, masks, etc

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