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 to_tensor() with pil_to_tensor() + convert_image_dtype() #4452

Merged

Conversation

prabhat00155
Copy link
Contributor

@prabhat00155 prabhat00155 commented Sep 20, 2021

This PR replaces uses of to_tensor() in reference scripts with a combination of pil_to_tensor() + convert_image_dtype(). to_tensor() has some implicit assumptions and is unclear with a number of issues reported with it.
cc @datumbox

@prabhat00155
Copy link
Contributor Author

Detection training logs with these changes:
detection_training_run.txt

@prabhat00155 prabhat00155 marked this pull request as ready for review September 21, 2021 13:45
@prabhat00155
Copy link
Contributor Author

Segmentation training logs with these changes:
segmentation_training_run.txt

@datumbox
Copy link
Contributor

@prabhat00155 Can you provide validation runs for pretrained models? We need to ensure that before/after all metrics remain the same for all models.

@prabhat00155
Copy link
Contributor Author

fasterrcnn_resnet50_fpn:
Without changes:

IoU metric: bbox
 Average Precision  (AP) @[ IoU=0.50:0.95 | area=   all | maxDets=100 ] = 0.369
 Average Precision  (AP) @[ IoU=0.50      | area=   all | maxDets=100 ] = 0.585
 Average Precision  (AP) @[ IoU=0.75      | area=   all | maxDets=100 ] = 0.396
 Average Precision  (AP) @[ IoU=0.50:0.95 | area= small | maxDets=100 ] = 0.212
 Average Precision  (AP) @[ IoU=0.50:0.95 | area=medium | maxDets=100 ] = 0.403
 Average Precision  (AP) @[ IoU=0.50:0.95 | area= large | maxDets=100 ] = 0.482
 Average Recall     (AR) @[ IoU=0.50:0.95 | area=   all | maxDets=  1 ] = 0.307
 Average Recall     (AR) @[ IoU=0.50:0.95 | area=   all | maxDets= 10 ] = 0.485
 Average Recall     (AR) @[ IoU=0.50:0.95 | area=   all | maxDets=100 ] = 0.509
 Average Recall     (AR) @[ IoU=0.50:0.95 | area= small | maxDets=100 ] = 0.317
 Average Recall     (AR) @[ IoU=0.50:0.95 | area=medium | maxDets=100 ] = 0.544
 Average Recall     (AR) @[ IoU=0.50:0.95 | area= large | maxDets=100 ] = 0.649

With changes:

IoU metric: bbox
 Average Precision  (AP) @[ IoU=0.50:0.95 | area=   all | maxDets=100 ] = 0.369
 Average Precision  (AP) @[ IoU=0.50      | area=   all | maxDets=100 ] = 0.585
 Average Precision  (AP) @[ IoU=0.75      | area=   all | maxDets=100 ] = 0.396
 Average Precision  (AP) @[ IoU=0.50:0.95 | area= small | maxDets=100 ] = 0.212
 Average Precision  (AP) @[ IoU=0.50:0.95 | area=medium | maxDets=100 ] = 0.403
 Average Precision  (AP) @[ IoU=0.50:0.95 | area= large | maxDets=100 ] = 0.482
 Average Recall     (AR) @[ IoU=0.50:0.95 | area=   all | maxDets=  1 ] = 0.307
 Average Recall     (AR) @[ IoU=0.50:0.95 | area=   all | maxDets= 10 ] = 0.485
 Average Recall     (AR) @[ IoU=0.50:0.95 | area=   all | maxDets=100 ] = 0.509
 Average Recall     (AR) @[ IoU=0.50:0.95 | area= small | maxDets=100 ] = 0.317
 Average Recall     (AR) @[ IoU=0.50:0.95 | area=medium | maxDets=100 ] = 0.544
 Average Recall     (AR) @[ IoU=0.50:0.95 | area= large | maxDets=100 ] = 0.649

@prabhat00155
Copy link
Contributor Author

fcn_resnet50:
Without changes:

Test: Total time: 0:00:48
global correct: 91.4
average row correct: ['94.0', '80.9', '73.9', '72.3', '49.9', '56.9', '81.0', '66.5', '88.6', '43.2', '75.7', '67.6', '80.9', '84.3', '81.2', '90.9', '49.0', '80.4', '61.2', '77.7', '69.0']
IoU: ['90.3', '72.6', '61.2', '61.6', '42.7', '46.9', '73.0', '53.7', '80.3', '33.6', '64.1', '33.9', '61.2', '72.0', '73.6', '80.7', '29.1', '63.5', '48.2', '71.2', '56.3']
mean IoU: 60.5

With changes:

Test: Total time: 0:00:49
global correct: 91.4
average row correct: ['94.0', '80.9', '73.9', '72.3', '49.9', '56.9', '81.0', '66.5', '88.6', '43.2', '75.7', '67.6', '80.9', '84.3', '81.2', '90.9', '49.0', '80.4', '61.2', '77.7', '69.0']
IoU: ['90.3', '72.6', '61.2', '61.6', '42.7', '46.9', '73.0', '53.7', '80.3', '33.6', '64.1', '33.9', '61.2', '72.0', '73.6', '80.7', '29.1', '63.5', '48.2', '71.2', '56.3']
mean IoU: 60.5

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! I love to see to_tensor() go. Thanks Prabhat. :)

@prabhat00155 prabhat00155 merged commit c6a03c7 into pytorch:main Sep 21, 2021
@prabhat00155 prabhat00155 deleted the prabhat00155/replace_to_tensor branch September 21, 2021 20:44
@datumbox
Copy link
Contributor

@prabhat00155 Some ToTensor() survived the extermination. :(

This is the class form not the functional part of the implementation. If you search for ToTensor() in the code you will find lots of references on docstrings as well.

What that intentionally left out? It would be awesome if we could replace them as well.

@prabhat00155
Copy link
Contributor Author

@prabhat00155 Some ToTensor() survived the extermination. :(

This is the class form not the functional part of the implementation. If you search for ToTensor() in the code you will find lots of references on docstrings as well.

What that intentionally left out? It would be awesome if we could replace them as well.

Okay, will do that.

@datumbox
Copy link
Contributor

Might require additional validation runs. Happy to review the PR if you tag me.

@prabhat00155
Copy link
Contributor Author

Might require additional validation runs. Happy to review the PR if you tag me.

Here is the PR: #4481

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

Reviewed By: datumbox

Differential Revision: D31268041

fbshipit-source-id: c1300d28c4474a95db7fe902bfe694e08ae7db39
@yiwen-song
Copy link
Contributor

Hey @prabhat00155, thanks for doing this! Do we also have plan for replacing all the to_tensor() used in references/classification/presets.py?

@datumbox
Copy link
Contributor

@sallysyw It has already been replaced on a previous PR.

@yiwen-song
Copy link
Contributor

ohhh right! My bad. Thanks for pointing it out.

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