Hi @federicopozzi33!

Thank you for your pull request and welcome to our community.

Action Required

In order to merge any pull request (code, docs, etc.), we require contributors to sign our Contributor License Agreement, and we don't seem to have one on file for you.

Process

In order for us to review and merge your suggested changes, please sign at https://code.facebook.com/cla. If you are contributing on behalf of someone else (eg your employer), the individual CLA may not be sufficient and your employer may need to sign the corporate CLA.

Once the CLA is signed, our tooling will perform checks and validations. Afterwards, the pull request will be tagged with CLA signed. The tagging process may take up to 1 hour after signing. Please give it that time before contacting us about it.

If you have received this in error or have any questions, please contact us at [email protected]. Thanks!

💊 CI failures summary and remediations

As of commit 52dc452 (more details on the Dr. CI page):

💚 💚 Looks good so far! There are no failures yet. 💚 💚

I have some doubts:

The first one is about testing.
I saw only smoke tests for other transformations and no particular tests for random transformations in test_prototype_transforms.py

How are expected to be tested the random transforms?

I would mock/patch the torch.rand to control randomness. Another solution could be to add two tests, with p=0 and p=1, and check the expected result.

The second one is about the HorizontalFlip transform.

The _transform method is not handling inputs of type features.SegmentationMask.
IMO, the horizontal flip is performed the same way for features.Image and features.SegmentationMask.

So, the _transform method could be:

class HorizontalFlip(Transform):
    def _transform(self, input: Any, params: Dict[str, Any]) -> Any:
        if isinstance(input, features.Image):
            output = F.horizontal_flip_image_tensor(input)
            return features.Image.new_like(input, output)
        elif isinstance(input, features.SegmentationMask):
            output = F.horizontal_flip_image_tensor(input)
            return features.SegmentationMask.new_like(input, output)
...

Thank you for signing our Contributor License Agreement. We can now accept your code for this (and any) Meta Open Source project. Thanks!

@federicopozzi33 Very good points. I think we should definitely look for a better testing strategy for the prototype area. I also agree that SegmentationMask should be easily supported. Perhaps we should incorporate your proposal in this PR.

@pmeier We shouldn't have both a HorizontalFlip and a RandomHorizontalFlip. I believe this was previously discussed but missed during the ticket creation (worth checking the existing tickets to avoid other issues)? I think the right approach here is to merge @federicopozzi33's RandomHorizontalFlip implementation in the existing HorizontalFlip and just rename the class.

To add to Vasilis comments:

I would mock/patch the torch.rand to control randomness. Another solution could be to add two tests, with p=0 and p=1, and check the expected result.

Mocking randomness can become really weird really fast. Since our transformations support the p parameter, I would go for the tests you proposed. For good measure we can throw in some smoke test for p not in {0, 1}. Not sure if it is worth it though.

The _transform method is not handling inputs of type features.SegmentationMask.
IMO, the horizontal flip is performed the same way for features.Image and features.SegmentationMask.

I agree with the sentiment that we should support segmentation masks here. They haven't got any love beyond a few examples yet, so there will probably be more transforms were we could add this support.

That being said, I think we need to decide if we want to have a separate kernel for them or not. Right now we have

which is a very thin wrapper. Given that on the "tensor level" images and segmentation masks are basically the same and the distinction only comes on the "feature level", I think we should remove the kernel here and handle this on the transform as proposed by @federicopozzi33.

@federicopozzi33 Very good points. I think we should definitely look for a better testing strategy for the prototype area. I also agree that SegmentationMask should be easily supported. Perhaps we should incorporate your proposal in this PR.

@pmeier We shouldn't have both a HorizontalFlip and a RandomHorizontalFlip. I believe this was previously discussed but missed during the ticket creation (worth checking the existing tickets to avoid other issues)? I think the right approach here is to merge @federicopozzi33's RandomHorizontalFlip implementation in the existing HorizontalFlip and just rename the class.

I agree with you. I did as you proposed.

To add to Vasilis comments:

I would mock/patch the torch.rand to control randomness. Another solution could be to add two tests, with p=0 and p=1, and check the expected result.

Mocking randomness can become really weird really fast. Since our transformations support the p parameter, I would go for the tests you proposed. For good measure we can throw in some smoke test for p not in {0, 1}. Not sure if it is worth it though.

I added some unit tests to test all cases when the transform is always performed (p=1.0).
IMO, I would create separate test files for each transform, or at least a specific test class as I did.
I chose to create some very specific helper methods to generate simple data tensors: this should make tests more readable.

One test case is missing, the fallback case, cause it sounds a little strange to me the following behavior:

actual = transforms.RandomHorizontalFlip(p=1.0)(1.0)
assert actual == 1.0

Anyway, before adding quite similar tests for the missing case (p=0), I would appreciate some feedback/advice.
I think they can be refactored/improved a little bit more, to make them at least more compact. I would have used parametrize decorator from pytest, but it seems that I can't call instance methods to list the argument sets, like this:

class TestRandomHorizontalFlip:
    def input_tensor(self):
        pass

    def expected_tensor(self):
        pass

    @pytest.mark.parametrize("input, expected", [(self.input_tensor(), self.expected_tensor()), ...])
    def test_p_1(self, input, expected):
        pass

Another option might be to use fixtures.

The _transform method is not handling inputs of type features.SegmentationMask.
IMO, the horizontal flip is performed the same way for features.Image and features.SegmentationMask.

I agree with the sentiment that we should support segmentation masks here. They haven't got any love beyond a few examples yet, so there will probably be more transforms were we could add this support.

That being said, I think we need to decide if we want to have a separate kernel for them or not. Right now we have

vision/torchvision/prototype/transforms/functional/_geometry.py

Lines 62 to 65 in d8654bb

	def resize_segmentation_mask(
	segmentation_mask: torch.Tensor, size: List[int], max_size: Optional[int] = None
	) -> torch.Tensor:
	return resize_image_tensor(segmentation_mask, size=size, interpolation=InterpolationMode.NEAREST, max_size=max_size)

which is a very thin wrapper. Given that on the "tensor level" images and segmentation masks are basically the same and the distinction only comes on the "feature level", I think we should remove the kernel here and handle this on the transform as proposed by @federicopozzi33.

Yeah, I agree with you.

@datumbox, @pmeier

Let me add tests for p=0, then you can do the final review and approve it!

Hey @pmeier!

You merged this PR, but no labels were added. The list of valid labels is available at https://github.com/pytorch/vision/blob/main/.github/process_commit.py