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

ObjectDetectionTask: increase test coverage for torchvision 0.14+ #1739

Merged

Conversation

isaaccorley
Copy link
Collaborator

@isaaccorley isaaccorley commented Nov 22, 2023

The try/except in the ObjectDetection trainer's validation step was suppressing the following errors which drives this PR. These errors are caused by the use of torch.rand to generate dummy bounding box data which results in some cases where xmax < xmin and/or ymax < ymin which results in the bounding box plotting throwing a ValueError

FAILED tests/trainers/test_detection.py::TestObjectDetectionTask::test_trainer[True-faster-rcnn-nasa_marine_debris] - ValueError: Boxes need to be in (xmin, ymin, xmax, ymax) format. Use torchvision.ops.box_convert to convert them
FAILED tests/trainers/test_detection.py::TestObjectDetectionTask::test_trainer[True-fcos-nasa_marine_debris] - ValueError: Boxes need to be in (xmin, ymin, xmax, ymax) format. Use torchvision.ops.box_convert to convert them
FAILED tests/trainers/test_detection.py::TestObjectDetectionTask::test_trainer[True-retinanet-nasa_marine_debris] - ValueError: Boxes need to be in (xmin, ymin, xmax, ymax) format. Use torchvision.ops.box_convert to convert them

Additionally, this PR adds a missing test for covering the case where fig returned by the plot method is None because the dataset has no plot method.

@isaaccorley isaaccorley changed the title Trainers validation step coverage Trainers validation step plot coverage Nov 22, 2023
@github-actions github-actions bot added the testing Continuous integration testing label Nov 22, 2023
@isaaccorley isaaccorley reopened this Nov 22, 2023
Copy link
Collaborator

@adamjstewart adamjstewart left a comment

Choose a reason for hiding this comment

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

Curious why we never encountered this issue before, but it fails consistently every time in your PR that bumps torch/torchvision versions. Did something change?

There's a lot of code duplication here. Can we:

  1. Move plot_no_rgb and no_plot_method to tests/trainers/conftest.py so they can be shared?
  2. Combine test_no_rgb and test_no_plot_method into a single parameterized test_plot method?

@adamjstewart adamjstewart added this to the 0.5.2 milestone Nov 24, 2023
@isaaccorley
Copy link
Collaborator Author

@adamjstewart, I may not have time to finish this in the next few days since I'm traveling. Not sure if you want to finish it up.

@adamjstewart
Copy link
Collaborator

Did something change?

Something did indeed change: pytorch/vision#6123

So we used to be generating (10, 4) random floats. The chances that any particular tuple satisfies xmax > xmin AND ymax > ymin is 0.25. The chances that all 10 tuples satisfy this is $0.25^{10} = 9.5^{-7}$. This explains why the tests basically always skipped that line in torchvision 0.14+.

@adamjstewart adamjstewart changed the title Trainers validation step plot coverage ObjectDetectionTask: increase test coverage for torchvision 0.14+ Nov 25, 2023
Copy link
Collaborator

@adamjstewart adamjstewart left a comment

Choose a reason for hiding this comment

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

Thanks for finding this bug! Glad it was only in our testing code and not in the core library.

@adamjstewart adamjstewart enabled auto-merge (squash) November 25, 2023 16:52
@adamjstewart adamjstewart merged commit cd1be7a into microsoft:main Nov 25, 2023
21 checks passed
@isaaccorley isaaccorley deleted the trainers/object-detection-coverage branch December 6, 2023 01:30
isaaccorley added a commit that referenced this pull request Mar 2, 2024
)

* fix test coverage in trainers validation step

* add tests for multilabelclassificationtask

* Minimal bug fix for now

* Simpler random code

---------

Co-authored-by: Adam J. Stewart <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
testing Continuous integration testing
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants