Skip to content

Fixed issues with dtype in geom functional transforms v2 #7211

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

Merged

Conversation

vfdev-5
Copy link
Collaborator

@vfdev-5 vfdev-5 commented Feb 9, 2023

Description story:

Philip (@pmeier) tried to check if other floating types are working for the transforms: f16, f64 etc. here: #7195

It seems that f16 is not even working for stable one (ref). But for f64 we had few issues that this PR is supposed to fix.

Some transforms can't work properly for f64, e.g. perspective. Jit is inconsitent etc

cc @bjuncek @pmeier

@NicolasHug NicolasHug mentioned this pull request Feb 10, 2023
49 tasks
) -> torch.Tensor:

fp = img.dtype == grid.dtype
Copy link
Member

Choose a reason for hiding this comment

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

Could you please explain why we're sure that img.dtype is float iff it's the same as the grid dtype? Can't we just use is_floating_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.

yes, we can use is_floating_dtype. I used a context knowledge that grid should have float dtype

@NicolasHug
Copy link
Member

Thanks for the PR @vfdev-5 . It's not really clear to me which bugs this issue is trying to address. Could you help me understand?

@pmeier
Copy link
Collaborator

pmeier commented Feb 10, 2023

@NicolasHug This addresses the second point in #7159 (comment). TL;DR You saw tests for elastic completely failing and this happened because we failed to consolidate the dtypes of the image and grid that were passed to grid_sample. This PR (hopefully; I haven't reviewed yet either) fixes this and expands our tests so this doesn't happen again.

@pmeier pmeier self-requested a review February 10, 2023 14:52
@vfdev-5 vfdev-5 requested a review from pmeier February 13, 2023 11:25
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 @vfdev-5!

@vfdev-5 vfdev-5 merged commit af7c6c0 into pytorch:main Feb 13, 2023
@vfdev-5 vfdev-5 deleted the proto-apply-grid-transform-restore-dtype-cast branch February 13, 2023 13:33
@github-actions
Copy link

Hey @vfdev-5!

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 Mar 28, 2023
)

Summary: Co-authored-by: Philip Meier <[email protected]>

Reviewed By: vmoens

Differential Revision: D44416263

fbshipit-source-id: 4bf99470ac106dd8d1c15fa2e217e865508650d4
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.

4 participants