Add scale option to ToDtype. Remove ConvertDtype. by NicolasHug · Pull Request #7759 · pytorch/vision

NicolasHug · 2023-07-25T16:54:44Z

Closes #7756

this PR:

adds a scale parameter to ToDtype, which only affects images or videos
removes ConvetDtype to keep ConvertImageDtype. ConvertImageDtype now only supports images, not videos.
removes all dispatchers / kernels associated with convert_.* and replace with to_dtype* dispatchers / kernels.
When passing ToDtype(torch.float32) i.e. the dtype parameter is just a dtype, not a dict, we only convert images and videos - this is for BC with ConvertImageDtype and reduces adoption friction

(I may have removed too much stuff on the existing tests, we'll see with the CI)

cc @vfdev-5

pytorch-bot · 2023-07-25T16:54:47Z

🔗 Helpful Links

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

📄 Preview Python docs built from this PR

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

❌ 23 New Failures

As of commit dd903a8:

NEW FAILURES - The following jobs have failed:

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

torchvision/transforms/v2/_misc.py

test/test_transforms_v2_refactored.py

pmeier

I have a bunch of comments, but overall looks solid.

test/test_transforms_v2_refactored.py

pmeier · 2023-07-26T07:00:49Z

torchvision/transforms/v2/functional/_meta.py


-def convert_dtype_image_tensor(image: torch.Tensor, dtype: torch.dtype = torch.float) -> torch.Tensor:
+def to_dtype_image_tensor(image: torch.Tensor, dtype: torch.dtype = torch.float, scale: bool = False) -> torch.Tensor:
+    if not scale:


Should this come after the dtype check? Functionally, this is a no-op here in case image.dtype == dtype, but it is still an unnecessary call.

if image.dtype == dtype: return image elif not scale: return image.to(dtype)

makes the behavior a little more clear and should be minimally more performant.

I'll do it but TBH I don't really follow the reasoning behind the change

The elif part actually adds confusion to me because it suggests these 2 blocks are related when in reality they're really not

Re-reading the original implementation: I was wrong. Since we already return'ed in both branches, this should have no effect on the performance. Thus, feel free to revert to what you had.

Still, I would put the dtype check at the top, because that is the "more important" one. Again, just style / personal preference. Ignore if you feel different.

The elif part actually adds confusion to me because it suggests these 2 blocks are related when in reality they're really not

Fair enough. Happy with a second if as well.

torchvision/transforms/v2/_misc.py

torchvision/transforms/v2/functional/_meta.py

test/test_transforms_v2_refactored.py

pmeier · 2023-07-26T07:50:01Z

test/test_transforms_v2_refactored.py

    check_cuda_vs_cpu=True,
    check_scripted_vs_eager=True,
    check_batched_vs_unbatched=True,
+    expect_same_dtype=True,


Not against it, but are we expecting more kernels to set this to False?

I don't think so (more precisely: I don't know). We still want the rest of the checks to be done for ToDtype though. Is there a better way than to add a parameter?

Not really. Since this will be the only the kernel that ever needs this, we could implement a custom check_kernel inside the test class. Basically copy-pasting the current function, but add the new parameter as well as stripping everything that is more generic, but not needed in this specific case. This would keep the check_kernel signature clean since it already has quite a few parameters.

Kinda torn on this. Up to you. I'm ok with both.

I have a slight preference for adding the parameter, because otherwise we'd have to change both implementation of check_kernel if we ever needed to update it.
(looks like I'm advocating for this single entry point after all :p )

test/test_transforms_v2_refactored.py

torchvision/transforms/v2/functional/_meta.py

vfdev-5 · 2023-07-26T14:26:17Z

torchvision/transforms/v2/_misc.py

+    def __init__(self, dtype: Union[torch.dtype, Dict[Type, Optional[torch.dtype]]], scale: bool = False) -> None:
        super().__init__()
-        if not isinstance(dtype, dict):
-            dtype = _get_defaultdict(dtype)


Removing this behaviour we would like to go away from defaultdict(lambda: dtype) ?

Yes, this is one of the goals #7756 (comment). It basically just gets replaced by {"others": dtype}:

no need to know what a defaultdict is

no need to import defaultdict

no need to know what a lambda is

no need to understand how defaultdict and lambda interact

But should not this be done in a separate PR and everywhere it is used ?

If this gets accepted here I was going to submit a follow-up PR to do the same changes for the fill parameters (which is probably much more work). Are there other places where this pattern is used?

OK, sounds good to me.

PermuteDimensions, TransposeDimensions and _setup_fill_arg

Co-authored-by: vfdev <vfdev.5@gmail.com>

NicolasHug · 2023-07-27T08:44:59Z

test/test_transforms_v2_refactored.py

+    @pytest.mark.parametrize("output_dtype", [torch.float32, torch.float64, torch.uint8])
+    @pytest.mark.parametrize("device", cpu_and_cuda())
+    @pytest.mark.parametrize("scale", (True, False))
+    @pytest.mark.parametrize("as_dict", (True, False))


I added this because when the dtype parameters isn't a dict and when the input is a single bbox or a single mask (as is the case here), the input just gets passed-through. I'm converting the dtype into a dict so that we also test the rest of the code-paths for those single bboxes and masks. Not incredibly critical, just for coverage.

pmeier

Some comments, but nothing blocking. LGTM if CI is green. Thanks Nicolas!

pmeier · 2023-07-27T08:47:51Z

test/test_transforms_v2_refactored.py

    check_cuda_vs_cpu=True,
    check_scripted_vs_eager=True,
    check_batched_vs_unbatched=True,
+    expect_same_dtype=True,


Not really. Since this will be the only the kernel that ever needs this, we could implement a custom check_kernel inside the test class. Basically copy-pasting the current function, but add the new parameter as well as stripping everything that is more generic, but not needed in this specific case. This would keep the check_kernel signature clean since it already has quite a few parameters.

Kinda torn on this. Up to you. I'm ok with both.

test/test_transforms_v2_refactored.py

pmeier · 2023-07-27T08:53:21Z

test/test_transforms_v2_refactored.py

+            return inpt.max() <= 1
+
+        H, W = 10, 10
+        sample = {


Should we also throw a video in there?

We do (from the parametrization), although I kept img as a name which I agree is confusing. I'll use inpt instead

Thanks, I missed that.

test/test_transforms_v2_refactored.py

pmeier · 2023-07-27T08:56:56Z

test/test_transforms_v2_refactored.py

+            make_video,
+        ),
+    )
+    def test_behaviour(self, make_input):


This test is huge. Should we maybe split it into multiple ones?

Co-authored-by: Philip Meier <github.pmeier@posteo.de>

NicolasHug · 2023-07-27T12:44:41Z

Tests are green in 92f2588 except for the linter which I just fixed. Merging, thanks for the reviews!

github-actions · 2023-07-27T12:45:47Z

Hey @NicolasHug!

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

Reviewed By: matteobettini Differential Revision: D48642282 fbshipit-source-id: 95a2eea16407f17e1ebeb386cd5e2618a105450f Co-authored-by: vfdev <vfdev.5@gmail.com> Co-authored-by: Philip Meier <github.pmeier@posteo.de>

Add scale option to ToDtype. Remove ConvertDtype.

45e0e2e

facebook-github-bot added the cla signed label Jul 25, 2023

NicolasHug commented Jul 25, 2023

View reviewed changes

torchvision/transforms/v2/_misc.py Outdated Show resolved Hide resolved

test/test_transforms_v2_refactored.py Outdated Show resolved Hide resolved

test/test_transforms_v2_refactored.py Outdated Show resolved Hide resolved

pmeier reviewed Jul 26, 2023

View reviewed changes

NicolasHug added 3 commits July 26, 2023 10:48

Address comments

9ac672b

Add back comment

c3f71aa

Let non-dict dtype only care about images and videos

00aef9b

vfdev-5 reviewed Jul 26, 2023

View reviewed changes

NicolasHug and others added 3 commits July 27, 2023 09:28

Update torchvision/transforms/v2/functional/_meta.py

4afaee4

Co-authored-by: vfdev <vfdev.5@gmail.com>

Merge branch 'main' of github.com:pytorch/vision into dtypeeeeeeeeeeeee

64bfb7d

Add docs

c4224b3

NicolasHug commented Jul 27, 2023

View reviewed changes

Minor stuff

134deb5

pmeier approved these changes Jul 27, 2023

View reviewed changes

NicolasHug and others added 2 commits July 27, 2023 10:14

Update test/test_transforms_v2_refactored.py

760d014

Co-authored-by: Philip Meier <github.pmeier@posteo.de>

img -> inpt, split tests

ea7c1e2

This was referenced Jul 27, 2023

ToDype / ConvertImageDtype file location #7764

Closed

Allow "others" key as a catch-all in dictionary parameters #7765

Closed

NicolasHug added 3 commits July 27, 2023 10:54

mypy

fb288d2

Remove some old tests

3d0d5d9

Hopefully fix some tests

92f2588

NicolasHug mentioned this pull request Jul 27, 2023

Test mode dtypes in the ToDtype tests #7767

Open

Fix linter

dd903a8

NicolasHug merged commit 1402eb8 into pytorch:main Jul 27, 2023

NicolasHug added enhancement module: transforms labels Jul 27, 2023

lyuwenyu mentioned this pull request Aug 31, 2023

Issue with training on user data lyuwenyu/RT-DETR#52

Closed

Conversation

NicolasHug commented Jul 25, 2023 • edited by pytorch-bot bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

pytorch-bot bot commented Jul 25, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

🔗 Helpful Links

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

❌ 23 New Failures

Uh oh!

Uh oh!

Uh oh!

Uh oh!

pmeier left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

NicolasHug Jul 26, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

NicolasHug Jul 26, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

pmeier left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

NicolasHug commented Jul 27, 2023

Uh oh!

github-actions bot commented Jul 27, 2023

Uh oh!

Reviewers

Assignees

NicolasHug commented Jul 25, 2023 •

edited by pytorch-bot bot

Loading

pytorch-bot bot commented Jul 25, 2023 •

edited

Loading

NicolasHug Jul 26, 2023 •

edited

Loading

NicolasHug Jul 26, 2023 •

edited

Loading