Implement Flip transforms with CVCUDA backend

[zy1git]

Summary:
Implemented _horizontal_flip_image_cvcuda and _vertical_flip_image_cvcuda kernels using cvcuda.flip operator. The kernels are automatically registered when CVCUDA is available and route cvcuda.Tensor inputs appropriately.

Test Plan:

• Added test_functional_cvcuda and test_image_correctness_cvcuda tests
• Verified parity between PyTorch and CVCUDA implementations
• All tests pass with CVCUDA backend

[pytorch-bot]

🔗 Helpful Links

🧪 See artifacts and rendered test results at hud.pytorch.org/pr/pytorch/vision/9277

• 📄 Preview Python docs built from this PR

Note: Links to docs will display an error until the docs builds have been completed.

❗ 2 Active SEVs

There are 2 currently active SEVs. If your PR is affected, please view them below:

• Dr CI is temporarily not working due to API fairewall
• Pytorch CI is paused for the time being

This comment was automatically generated by Dr. CI and updates every 15 minutes.

[justincdavis]

@zy1git What is the strategy for creating the tests for the transforms with CV-CUDA backends? Do we want to have all the tests live entirely inside the existing classes or make a new class?

The PRs for gaussian_blur, normalize, and to_dtype I made all use new classes, but I can switch it be more centralized.

[AntoineSimoulin]

Thanks a lot for submitting this PR! This is looking good. I added some comments to make sure we have an extensive test coverage:)

[NicolasHug]

@justincdavis replying to your question in #9277 (comment): we prefer centralizing the tests in the existing test class. The idea is that, as much as possible, we'd just add CV-CUDA as a parametrization entry with pytest.mark.parametrize to the existing tests. Antoine and I left a few comments related to that above. Does that make sense?

[justincdavis]

@NicolasHug Makes sense! I will follow the comments you and Antoine left on this PR

[AntoineSimoulin]

Thanks a lot for addressing the initial comments! I left some final adjustments to make. Let's also make sure linting and tests are passing!

[justincdavis]

@AntoineSimoulin @NicolasHug @zy1git

I would propose that we take an approach like this to simplify the comparison of CV-CUDA tensors with PIL references.

We would add the following function to common_utils.py

def cvcuda_to_pil_compatible_tensor(tensor):
    tensor = cvcuda_to_tensor(tensor)
    if tensor.ndim != 4:
        raise ValueError(f"CV-CUDA Tensor should be 4 dimensional. Got {tensor.ndim} dimensions.")
    if tensor.shape[0] != 1:
        raise ValueError(f"CV-CUDA Tensor should have batch dimension 1 for comparison with PIL.Image.Image. Got {tensor.shape[0]}.")
    return tensor.squeeze(0).cpu()

Then we would modify ImagePair as follows

class ImagePair(TensorLikePair):
    def __init__(
        self,
        actual,
        expected,
        *,
        mae=False,
        **other_parameters,
    ):
        if all(isinstance(input, PIL.Image.Image) for input in [actual, expected]):
            actual, expected = (to_image(input) for input in [actual, expected])
        elif CVCUDA_AVAILABLE and all(isinstance(input, _import_cvcuda().Tensor) for input in [actual, expected]):
            actual, expected = (cvcuda_to_tensor(input) for input in [actual, expected])
        elif CVCUDA_AVAILABLE and isinstance(actual, _import_cvcuda().Tensor) and isinstance(expected, PIL.Image.Image):
            actual = cvcuda_to_pil_compatible_tensor(actual)
            expected = to_image(expected)
        elif CVCUDA_AVAILABLE and isinstance(actual, _import_cvcuda().Tensor):
            actual = cvcuda_to_pil_compatible_tensor(actual)

        super().__init__(actual, expected, **other_parameters)
        self.mae = mae

Then when we compare the actual tensor (cvcuda.Tensor) to the expected (PIL.Image.Image), the conversion will be handled automatically for us.

Additionally, with the helper function cvcuda_to_pil_compatible_tensor, we can simplify the logic for handling CV-CUDA specific comparisions. For example, in TestRgbToGrayscale::test_image_correctness, the logic for handling CV-CUDA goes from:

if make_input is make_image_cvcuda:
        actual = F.cvcuda_to_tensor(actual).to(device="cpu")
        actual = actual.squeeze(0)
        # drop the batch dimension
        image = F.cvcuda_to_tensor(image).to(device="cpu")
        image = image.squeeze(0)

to

# here the conversion of actual is handled in either assert_close or assert_equal itself
if make_input is make_image_cvcuda:
        image = cvcuda_to_pil_compatible_tensor(image)

These changes are integrated in this PR

[NicolasHug]

Thanks for the great work @zy1git ! I made another pass.

[NicolasHug]

Above: this looks like a cosmetic change? Try to avoid that and revert the previous state, it's distracting when reviewing and it also affects git blame unnecessarily :)

[zy1git]

I think this cosmetic change happens after I use pre-commit hooks which run both ufmt and flake8 as described here

[NicolasHug]

Same here, this looks like a cosmetic change?

[NicolasHug]

This doesn't seem to be used, we can remove it.

[NicolasHug]

This seems unnecessary, we should be able to compare tensors where the batch dim is 1. Try to remove it, if it doesn't work for any reason let me know.

[NicolasHug]

EDIT: ah, OK, it's for when we compare a 3D PIL image to a 4D cvcuda tensor. That's... fine. Let's explain why then (addition in bold):

Remove batch dimension if it's 1 for easier comparison against 3D PIL images

[NicolasHug]

Let's use the _is_cvcuda_available() instead of a try/except. You can then call cvcuda = _import_cvcuda() which is safer and less surprising than a raw import cvcuda.

[NicolasHug]

there should be no need to manually convert anymore, assert_equal should be able to handle it. Also don't make raw cvcuda accesses! It would force a hard dependency :)

[NicolasHug]

revert both above, they seem unrelated and not needed.

[NicolasHug]

Noting that the above is a change of behavior: we used to convert both inputs, or none of them, which means we'd error when comparing a tensor to a PIL image. We now accept it. I think that's fine.

@justincdavis thanks for sharing your implementation in https://github.com/pytorch/vision/pull/9284/files#diff-f833e6bb21df531837a51e84306266c2f1f6d1565340a498095868e24b3f27de. I think you were being slightly more conservative in your added logic. Was there any particular edge-case you had in mind that you wanted to guard against?

[NicolasHug]

I think this should be

Suggested change

	if CVCUDA_AVAILABLE:
	_transformed_types = (torch.Tensor, PIL.Image.Image, cvcuda.Tensor)
	_transformed_types = _RandomApplyTransform._transformed_types + (is_cvcuda_tensor, )

where is_cvcuda_tensor is @justincdavis 's implementation from:
https://github.com/pytorch/vision/pull/9283/files#diff-6e892912803ab29861746f7118e9e462a5c9eaab50ad286903e87f09f6c44ff3R175

The function is great and it allows us to avoid the big if CVCUDA_AVAILABLE.

To follow-up from the discussion in https://github.com/pytorch/vision/pull/9277/files#r2566354008 about whether this _transformed_types should be in each child transform class or in the base Transform class: I think it should be in each child transform for now (as done here):

• we may have a to push a release where we won't have implemented cvcuda support for all transforms, so if this was in the base class then we'd be claiming cvcuda support for some transforms that actually do not support it
• we may publish new transforms in the future which won't immediately come with cvcuda support, causing the same kind of problem.

We could revisit this eventually. It might eventually be simpler to just have that once and for all in the base Transform class. But having it in each child classes isn't much work, and it makes it easier to track which ones support cvcuda, and which ones don't.