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

Replaced ToTensor with a combination of PILToTensor and ConvertImageDtype #4481

Merged
merged 4 commits into from
Sep 26, 2021

Conversation

prabhat00155
Copy link
Contributor

@prabhat00155 prabhat00155 commented Sep 24, 2021

Training logs before these changes:
training_log_no_changes.txt
Training logs after these changes:
training_log_after_changes.txt

cc @datumbox

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.

The changes LGTM, but there are additional references of ToTensor in other places. I think it's worth removing them from code usages and documentation.

Here are some examples:

>>> transforms.ToTensor(),

>>> transforms.ToTensor(),


Feel free to merge this PR if you want and open another one to address the rest, or do the changes here. Whatever works for you.

@prabhat00155
Copy link
Contributor Author

The changes LGTM, but there are additional references of ToTensor in other places. I think it's worth removing them from code usages and documentation.

Here are some examples:

>>> transforms.ToTensor(),

>>> transforms.ToTensor(),

Feel free to merge this PR if you want and open another one to address the rest, or do the changes here. Whatever works for you.

Thanks @datumbox! I already replaced T.ToTensor() in detection and segmentation to be a combination of pil_to_tensor() and convert_image_dtype() in #4452. I'll update the example usages under torchvision in a separate PR.

@datumbox
Copy link
Contributor

@prabhat00155 You are right, I missed the fact that we use a custom ToTensor class which internally calls the F.pil_to_tensor() and F.convert_image_dtype() methods.

How would you feel about replacing the custom ToTensor class from the segmentation/detection references with 2 similar wrapper classes called ConvertImageDtype and PILToTensor? This is just to reinforce the fact that ToTensor is not advised. It won't have any real effects on what you actually do. Let me know your thoughts.

@prabhat00155 prabhat00155 merged commit b7c2358 into pytorch:main Sep 26, 2021
@prabhat00155 prabhat00155 deleted the prabhat00155/remove_ToTensor branch September 26, 2021 17:10
@prabhat00155
Copy link
Contributor Author

@prabhat00155 You are right, I missed the fact that we use a custom ToTensor class which internally calls the F.pil_to_tensor() and F.convert_image_dtype() methods.

How would you feel about replacing the custom ToTensor class from the segmentation/detection references with 2 similar wrapper classes called ConvertImageDtype and PILToTensor? This is just to reinforce the fact that ToTensor is not advised. It won't have any real effects on what you actually do. Let me know your thoughts.

Yeah, sure. I can make that change.

facebook-github-bot pushed a commit that referenced this pull request Sep 30, 2021
…ertImageDtype (#4481)

Summary:
* Replaced ToTensor with a combination of PILToTensor and ConvertImageDtype

* Pass dtype

Reviewed By: datumbox

Differential Revision: D31268058

fbshipit-source-id: 17f7c3994ce202f6457e589fd986038da3532dee
husthyc added a commit that referenced this pull request Oct 22, 2021
…type (#4481)

* Replaced ToTensor with a combination of PILToTensor and ConvertImageDtype

* Pass dtype

[ghstack-poisoned]
husthyc added a commit that referenced this pull request Oct 22, 2021
…type (#4481)

* Replaced ToTensor with a combination of PILToTensor and ConvertImageDtype

* Pass dtype

[ghstack-poisoned]
cyyever pushed a commit to cyyever/vision that referenced this pull request Nov 16, 2021
…type (pytorch#4481)

* Replaced ToTensor with a combination of PILToTensor and ConvertImageDtype

* Pass dtype
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