Skip to content
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

Let Normalize() and RandomPhotometricDistort return datapoints instead of tensors #7113

Merged

Conversation

NicolasHug
Copy link
Member

@NicolasHug NicolasHug commented Jan 19, 2023

See #7092 (comment) for the rationale.

There are some TODO comments on which I'd love some input

cc @bjuncek @pmeier

output = inpt.wrap_like(inpt, output, color_space=datapoints.ColorSpace.OTHER) # type: ignore[arg-type]

elif isinstance(inpt, PIL.Image.Image):
if isinstance(orig_inpt, PIL.Image.Image):
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I suspect the original version had a bug: at this point inpt was always a Tensor because of line 122, and so the condition on line 130 was never True and this function would always return a Tensor even if a PIL image was passed

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good catch!

Copy link
Collaborator

@pmeier pmeier left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, thanks Nicolas!

test/test_prototype_transforms_functional.py Outdated Show resolved Hide resolved
torchvision/prototype/datapoints/_image.py Outdated Show resolved Hide resolved
@@ -82,6 +82,7 @@ def _transform(self, inpt: Any, params: Dict[str, Any]) -> Any:
return output


# TODO: Are there tests for this class?
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think so. We have a pretty good grip on the functional testing, but so far the transforms testing is the wild west. Seems we have forgotten this completely.

output = inpt.wrap_like(inpt, output, color_space=datapoints.ColorSpace.OTHER) # type: ignore[arg-type]

elif isinstance(inpt, PIL.Image.Image):
if isinstance(orig_inpt, PIL.Image.Image):
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good catch!

@NicolasHug NicolasHug merged commit d7e5b6a into pytorch:main Jan 20, 2023
@github-actions
Copy link

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

facebook-github-bot pushed a commit that referenced this pull request Jan 24, 2023
…ts instead of tensors (#7113)

Reviewed By: YosuaMichael

Differential Revision: D42706907

fbshipit-source-id: a7b7487ab8563f8a43a0ebb84df19579ccd35fe1
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants