Conversation
🔗 Helpful Links🧪 See artifacts and rendered test results at hud.pytorch.org/pr/pytorch/vision/7703
Note: Links to docs will display an error until the docs builds have been completed. ❌ 5 New FailuresAs of commit 95c80d2: NEW FAILURES - The following jobs have failed:
This comment was automatically generated by Dr. CI and updates every 15 minutes. |
|
|
||
|
|
||
| horizontal_flip_image_pil = _FP.hflip | ||
| def horizontal_flip_image_pil(image: PIL.Image.Image) -> PIL.Image.Image: |
There was a problem hiding this comment.
Without this thin wrapper, the dispatch test fails, because horizontal_flip_image_pil is never called. Only _FP.hflip is called.
The old test handle this by allowing the user to set another name for mocking, but I don't want to bring this complexity to the new tests. Since we need to have v2 "standalone" from v1 at some point anyway, might as well do it here now.
There was a problem hiding this comment.
I'm a bit concerned in general about modifying the code only to make tests easier. I guess that is OK in this case because it doesn't add much complexity to the code, but let's keep an eye on this
There was a problem hiding this comment.
Indeed. The issue here is that in the beginning of the v2 transform kernels, we had a lot of them just aliased to their v1 equivalent. The "old" v2 test framework accounted for that with
vision/test/transforms_v2_kernel_infos.py
Lines 42 to 44 in d814772
e.g.
vision/test/transforms_v2_kernel_infos.py
Lines 215 to 217 in d814772
The one above is obsolete now, since we removed the aliasing some time ago in #6983.
So basically here I'm just doing something we already planned to do in the first place, just a little earlier.
| @pytest.mark.parametrize("size", OUTPUT_SIZES) | ||
| @pytest.mark.parametrize( | ||
| "input_type_and_kernel", | ||
| ("input_type", "kernel"), |
There was a problem hiding this comment.
Just a QoL improvement that saves us a line in the test below.
| expected_bboxes = expected_bboxes[0] | ||
|
|
||
| return expected_bboxes | ||
| return torch.stack([transform(b) for b in bounding_box.reshape(-1, 4).unbind()]).reshape(bounding_box.shape) |
There was a problem hiding this comment.
IMO, the old logic was harder to parse. Basically all we are doing here is to break a batched tensor into its individual boxes, apply the helper to them, and reverse the process.
There was a problem hiding this comment.
bounding_box.reshape(-1, 4)
The only reason we need this reshape is because we may pass non-2D boxes (i.e. a single box as 1D tensor), right?
There was a problem hiding this comment.
Yup. We could factor this out into a decorator that we can put onto reference functions so they only need to handle the unbatched case. For now, we only have the affine bbox helper so I left it as is. Will look into the decorator again if we need it elsewhere.
NicolasHug
left a comment
NicolasHug
left a comment
There was a problem hiding this comment.
Thanks Philip, minor Qs ans suggestion but LGTM
Should we also add a helper to test randomness as done in test_randomness()?
| expected_bboxes = expected_bboxes[0] | ||
|
|
||
| return expected_bboxes | ||
| return torch.stack([transform(b) for b in bounding_box.reshape(-1, 4).unbind()]).reshape(bounding_box.shape) |
There was a problem hiding this comment.
bounding_box.reshape(-1, 4)
The only reason we need this reshape is because we may pass non-2D boxes (i.e. a single box as 1D tensor), right?
|
|
||
|
|
||
| class TestHorizontalFlip: | ||
| def _make_input(self, input_type, *, dtype=None, device="cpu", spatial_size=(17, 11), **kwargs): |
There was a problem hiding this comment.
is this mostly the same as the one in TestResize?
There was a problem hiding this comment.
Yes. If that's ok with you, I'll copy paste it for now until I have a handful transforms ported. If it turns out we never or rarely use something else, I'll factor it out as public function.
|
|
||
|
|
||
| horizontal_flip_image_pil = _FP.hflip | ||
| def horizontal_flip_image_pil(image: PIL.Image.Image) -> PIL.Image.Image: |
There was a problem hiding this comment.
I'm a bit concerned in general about modifying the code only to make tests easier. I guess that is OK in this case because it doesn't add much complexity to the code, but let's keep an eye on this
Reviewed By: vmoens Differential Revision: D47186579 fbshipit-source-id: 5077d10522cf36ba99aac3863cd55bb967eb8c89
3 participants
cc @vfdev-5