port RandomHorizontalFlip to prototype API

[federicopozzi33]

Closes: #5523

[facebook-github-bot]

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 cla@fb.com. Thanks!

[facebook-github-bot]

💊 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. 💚 💚

---

This comment was automatically generated by Dr. CI (expand for details).

Please report bugs/suggestions to the (internal) Dr. CI Users group.

Click here to manually regenerate this comment.

[federicopozzi33]

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)
...

[facebook-github-bot]

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

[datumbox]

@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.

[pmeier]

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

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.

[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]

The code looks good to me. I'll leave it to @pmeier to do the final review and approve.

Just one quick nit below:

[federicopozzi33]

@datumbox, @pmeier

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

[pmeier]

Hey @federicopozzi33, I added some more comments.

[pmeier]

I'm not sure we need specific values here. I think we should be good to have random image inputs and use torch.flip. I'm aware that this is the only call in the kernel

vision/torchvision/transforms/functional_tensor.py

Lines 126 to 129 in a8bde78

	def hflip(img: Tensor) -> Tensor:
	_assert_image_tensor(img)
	return img.flip(-1)

but this is also what I would use to produce a expected output if I didn't know the internals of the kernel. cc @NicolasHug for an opinion

[federicopozzi33]

I used specific values to have full control over the creation of the expecting result. I preferred to not use the torch.flip to not "mock" the internals of the transformation.

[NicolasHug]

cc @NicolasHug for an opinion

No strong opinion on my side for this specific transform. In some cases it's valuable to have a simple implementation of the transform that we're testing - it helps understanding what's going on with simpler code, and we can call it on arbitrary input. In the case of flip the transform is very elementary and doesn't need much explaining anyway.

If we're confident that this hard-coded input covers all of what we might want to test against, then that's fine.

[pmeier]

Thanks @federicopozzi33 for the continued work on this. I have some more comments on the tests, but otherwise we are good to go!

[datumbox]

The code on the transforms LGTM. Once @pmeier's comments on the tests are addressed we are good to merge.

[pmeier]

One minor suggestion. Otherwise LGTM! Thanks a lot for the patience @federicopozzi33!

[github-actions]

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