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.

Sign up for GitHub

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

Jump to bottom

Add tests and proper support for videos in ConvertImageDtype #6783

Merged
merged 10 commits into from
Oct 21, 2022
Merged

Add tests and proper support for videos in ConvertImageDtype #6783

merged 10 commits into from
Oct 21, 2022

Conversation

pmeier
Copy link
Collaborator

@pmeier pmeier commented Oct 18, 2022

#6724 shoehorned support for features.Video into ConvertImageDtype. The implementation is correct, but the naming is confusing since it explicitly mentions images, but now also supports videos.

Furthermore, we don't have a canonical dispatcher as we do for all other kernels that support for two or more types. Again, this works since videos can be treated as images here but makes the naming confusing, e.g. F.convert_image_dtype(video).

This PR does a few things:

  • rename ConvertImageDtype and convert_image_dtype to ConvertDtype and convert_dtype while keeping aliases for the former names
  • add convert_dtype_image_tensor and convert_dtype_video kernels and make convert_dtype a proper dispatcher of the former
  • Add tests for the functionals

@pmeier pmeier requested review from vfdev-5 and datumbox October 18, 2022 07:50
pmeier
pmeier commented Oct 18, 2022
View reviewed changes
torchvision/prototype/transforms/functional/_type_conversion.py Outdated
@@ -41,5 +41,3 @@ def to_image_tensor(image: Union[torch.Tensor, PIL.Image.Image, np.ndarray]) ->
# We changed the names to align them with the new naming scheme. Still, `to_pil_image` is
# prevalent and well understood. Thus, we just alias it without deprecating the old name.
to_pil_image = to_image_pil

convert_image_dtype = _F.convert_image_dtype
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This was in here by mistake I think. The old transform ConvertImageDtype as well as the other conversion transform functionals were already under the _meta namespace. Thus, I moved this there.

@vfdev-5
Copy link
Collaborator

vfdev-5 commented Oct 18, 2022

I think ConvertDtype can lead to other confusion that it would change dtype for bbox, labels and masks.

@pmeier
Copy link
Collaborator Author

pmeier commented Oct 18, 2022

I agree, naming is most pressing thing here. But then convert_image_dtype is not a good name to start with either. I thought about ConvertValueRange, but this brings its own set of problems. We could do ConvertImageOrVideoDtype, but this goes against our naming scheme. Open for discussions on this.

@vfdev-5
Copy link
Collaborator

vfdev-5 commented Oct 18, 2022

Maybe, emphasize somehow that we change dtype for sample (not targets) ?

@pmeier
Copy link
Collaborator Author

pmeier commented Oct 19, 2022

This PR conflicts with #6795. However, #6795 has (hopefully) no controversial changes so my vote is out for merging that one first and afterwards coming back to this.

datumbox
datumbox reviewed Oct 20, 2022
View reviewed changes
Copy link
Contributor

@datumbox datumbox left a comment

Choose a reason for hiding this comment

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

@pmeier Thanks for the work.

I agree with the direction of the PR. I have 2 concerns over the use of copy and the introduction of more methods in some of the _Feature subclasses.

Concerning the conflicts with other PRs, this PR looks like it adds more tests and aligns the API which is less dangerous than rewriting the convert_dtype_image_tensor method. How confident are you about the changes on the other PR? Do you think we can benefit by these extra tests and thus prioritise merging this PR first?

@pmeier
Merge branch 'main'
79b337d 
Conflicts:
	test/prototype_transforms_kernel_infos.py
	torchvision/prototype/transforms/functional/_type_conversion.py
@pmeier pmeier mentioned this pull request Oct 20, 2022
Merged
pmeier added 3 commits October 21, 2022 14:57
@pmeier
Merge branch 'main'
55bf503 
Conflicts:
	torchvision/prototype/features/_image.py
	torchvision/prototype/features/_video.py
	torchvision/prototype/transforms/__init__.py
	torchvision/prototype/transforms/_meta.py
	torchvision/prototype/transforms/functional/_meta.py
@pmeier
cleanup
beedf49
@pmeier
fix typo
6c506fc
@pmeier pmeier requested a review from datumbox October 21, 2022 13:12
datumbox
datumbox approved these changes Oct 21, 2022
View reviewed changes
Copy link
Contributor

@datumbox datumbox 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!

@pmeier pmeier merged commit 9c11293 into pytorch:main Oct 21, 2022
@pmeier pmeier deleted the test-convert-image-dtype branch October 21, 2022 19:04
@pmeier pmeier mentioned this pull request Oct 25, 2022
Merged
facebook-github-bot pushed a commit that referenced this pull request Oct 27, 2022
@datumbox @facebook-github-bot
[fbsync] Add tests and proper support for videos in `ConvertImageDtyp…
08758ca 
…e` (#6783)

Summary:
* add KernelInfo

* split dtype and device consistency tests

* add proper support for video

* fix tests and add DispatcherInfo

* add aliases

* cleanup

* fix typo

Reviewed By: YosuaMichael

Differential Revision: D40722908

fbshipit-source-id: 36adda72819a12167ed12d27f6715a46c8ee9b56
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@datumbox datumbox datumbox approved these changes

@vfdev-5 vfdev-5 Awaiting requested review from vfdev-5

Assignees
No one assigned
Labels
cla signed module: transforms prototype
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@pmeier @vfdev-5 @datumbox @facebook-github-bot