-
Notifications
You must be signed in to change notification settings - Fork 6.9k
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
introduce heuristic for simple tensor handling of transforms v2 #7170
Conversation
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 Philip, gave it a quick look
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 Philip, some minor comments but it looks great
There are three transforms for which this heuristic is somewhat awkward and all for the same reason:
They all take an input argument that can be a dictionary and the transform selects the appropriate value based on the input type. For example: import torch
from torchvision.prototype import datapoints, transforms
sample = dict(
image=datapoints.Image(torch.randint(0, 256, (3, 32, 32), dtype=torch.uint8)),
boxes=datapoints.BoundingBox(torch.randint(0, 32, (5,)), format="xyxy", spatial_size=(32, 32)),
)
dtype = {
datapoints.Image: torch.float32,
datapoints.BoundingBox: torch.float64,
}
transform = transforms.ToDtype(dtype)
transformed_sample = transform(sample)
assert transformed_sample["image"].dtype is torch.float32
assert transformed_sample["boxes"].dtype is torch.float64
sample["tensor"] = torch.rand((3, 16, 16), dtype=torch.float16)
dtype[torch.Tensor] = torch.int32
transform = transforms.ToDtype(dtype)
transformed_sample = transform(sample)
assert transformed_sample["image"].dtype is torch.float32
assert transformed_sample["boxes"].dtype is torch.float64
assert transformed_sample["tensor"].dtype is torch.int32 # boom As shown above, the transform is not applied to the plain tensor according to the heuristic above. That is somewhat awkward since we specified it explicitly in parameter. The example above works on I guess one way to fix this is to disallow Thoughts? |
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 Philip, NIT but LGTM regardless
Sorry I had missed your comment above before I approved. This is unrelated to this issue, but I'm tempted to keep
Can we just raise a warning specific to
and still support the Tensor key if neither Image or Video are specified? |
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.
Still LGTM!
… v2 (#7170) Reviewed By: vmoens Differential Revision: D44416271 fbshipit-source-id: 20c92067665ea106550bc29947f2596a36000025
Addresses the thread in #6663 (comment).
cc @vfdev-5 @bjuncek