-
Notifications
You must be signed in to change notification settings - Fork 7.2k
Normalize CV-CUDA Backend #9279
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
base: main
Are you sure you want to change the base?
Conversation
🔗 Helpful Links🧪 See artifacts and rendered test results at hud.pytorch.org/pr/pytorch/vision/9279
Note: Links to docs will display an error until the docs builds have been completed. ✅ No FailuresAs of commit 974ffca with merge base 1e53952 ( This comment was automatically generated by Dr. CI and updates every 15 minutes. |
|
Hi @justincdavis! Thank you for your pull request and welcome to our community. Action RequiredIn 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. ProcessIn 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 If you have received this in error or have any questions, please contact us at cla@meta.com. Thanks! |
AntoineSimoulin
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hey @justincdavis, thanks for submitting the PR, this is looking good:) I left some minor changes. I think we mainly need to make sure the tests are passing when cvcuda is not installed!
test/test_transforms_v2.py
Outdated
| (F.normalize_video, tv_tensors.Video), | ||
| pytest.param( | ||
| F._misc._normalize_cvcuda, | ||
| _import_cvcuda().Tensor, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@justincdavis it seems that _import_cvcuda().Tensor is still raising an error if cvcuda is not installed. Maybe we can just use cvcuda.Tensor here and see if this works better?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you for pointing this out! I replaced the actual cvcuda.Tensor type with the string "cvcuda.Tensor", then inside the function we resolve the cvcuda.Tensor type if we have the corresponding string. LMK if this looks like a reasonable solution!
|
Following up from my comment in the
|
|
Hey @justincdavis, looking good to me. I don't think the failing test is related to this PR. Seems like a false positive alert to me! Can you sign our Contributor License Agreement (c.f. meta-cla bot comment in the discussion)? |
778ad32 to
d3ef0bd
Compare
6b7dd65 to
0f8910e
Compare
zy1git
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I left some comments. Some of them are referred to the merged PRs in the past three weeks.
NicolasHug
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks a lot for the PR @justincdavis , I left a review for @zy1git to address.
| if is_cvcuda: | ||
| assert_close(actual, expected, rtol=0, atol=1e-6) | ||
| else: | ||
| assert_equal(actual, expected) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm surprised atol=1e-6 is needed, I thought it was the default for float32. Let's try without it and see if the CI is happy?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I remove the rtol = 0, atol=1e-6 and the test passed. However, the default for assert_close is rtol = 1.3e-6, atol=1e-5, thus if the original implementation rtol=0, atol=1e-6 can pass the test, the default version definitely can pass.
I think atol = 1e-6 is more restricted threshold. I will change it to default in a new commit. Please let me know if you think we need to use the original implementation.
| if is_cvcuda: | ||
| image = F.cvcuda_to_tensor(image)[0].cpu() | ||
|
|
||
| expected = self._reference_normalize_image(image, mean=mean, std=std) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Note for self: double-checking that this is doing the right conversion. image is not a tensor and we're using _reference_normalize_image as the ref, we'll be comparing our cvcuda-tensor to this reference tensor. Seems OK.
test/test_transforms_v2.py
Outdated
| if is_cvcuda and dtype != torch.float32: | ||
| pytest.skip("CVCUDA only supports float32 for normalize") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Instead of a skip, this could be an xfail instead. See https://docs.pytest.org/en/stable/how-to/skipping.html
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I changed pytest.skip to pytest.xfail
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Note for self: need to check that all the relevant tests have been properly parametrized.
| # torchvision only supports uint and float, right now CV-CUDA doesnt expose float16, so only check 32 | ||
| # in the future add float16 once exposed in CV-CUDA | ||
| if not (image.dtype == cvcuda.Type.F32): | ||
| raise ValueError(f"Input tensor should be a float tensor. Got {image.dtype}.") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We should have a test that asserts non-float32 leads to an error and check the error message
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
zy1git
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I addressed comments by pushing a new commit. Feel free to take a look.
| if is_cvcuda: | ||
| assert_close(actual, expected, rtol=0, atol=1e-6) | ||
| else: | ||
| assert_equal(actual, expected) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I remove the rtol = 0, atol=1e-6 and the test passed. However, the default for assert_close is rtol = 1.3e-6, atol=1e-5, thus if the original implementation rtol=0, atol=1e-6 can pass the test, the default version definitely can pass.
I think atol = 1e-6 is more restricted threshold. I will change it to default in a new commit. Please let me know if you think we need to use the original implementation.
| # torchvision only supports uint and float, right now CV-CUDA doesnt expose float16, so only check 32 | ||
| # in the future add float16 once exposed in CV-CUDA | ||
| if not (image.dtype == cvcuda.Type.F32): | ||
| raise ValueError(f"Input tensor should be a float tensor. Got {image.dtype}.") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
test/test_transforms_v2.py
Outdated
| if is_cvcuda and dtype != torch.float32: | ||
| pytest.skip("CVCUDA only supports float32 for normalize") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I changed pytest.skip to pytest.xfail
Summary
This PR adds the CV-CUDA backend kernel for the
Normalizetransform.How to use
Run unit tests