-
Notifications
You must be signed in to change notification settings - Fork 7k
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
cleanup legacy transforms tests #8013
Conversation
🔗 Helpful Links🧪 See artifacts and rendered test results at hud.pytorch.org/pr/pytorch/vision/8013
Note: Links to docs will display an error until the docs builds have been completed. ✅ You can merge normally! (6 Unrelated Failures)As of commit c7f6efb with merge base af3077e (): FLAKY - The following jobs failed but were likely due to flakiness present on trunk:
BROKEN TRUNK - The following jobs failed but were present on the merge base:👉 Rebase onto the `viable/strict` branch to avoid these failures
This comment was automatically generated by Dr. CI and updates every 15 minutes. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I grew a little tired of all the porting. The last commit is 95% copy-pasting what we had before. I'll highlight a few things that might stand out. Otherwise, there is nothing new in this PR. Meaning, unless there is something that you absolutely cannot stand in our previous tests, let's please address this in follow-up PRs.
After all tests were ported, I acted on
vision/test/transforms_v2_legacy_utils.py
Lines 6 to 13 in ddfee23
The following legacy modules depend on this module | |
- test_transforms_v2_functional.py | |
- test_transforms_v2_consistency.py | |
- test_transforms_v2.py | |
When all the logic is ported from the files above to test_transforms_v2_refactored.py, delete | |
all the legacy modules including this one and drop the _refactored prefix from the name. |
and this is the cause of this massive diff.
test/test_transforms_v2.py
Outdated
@@ -185,7 +5388,7 @@ def test__transform(self, mocker): | |||
|
|||
size = (32, 24) | |||
image = make_image(size) | |||
bboxes = make_bounding_boxes(format="XYXY", canvas_size=size, batch_dims=(6,)) | |||
bboxes = make_bounding_boxes(format="XYXY", canvas_size=size, num_objects=6) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Here
([0, 0, 10, 10], True), | ||
([1, 1, 30, 20], True), | ||
] | ||
class TestSanitizeBoundingBoxes: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The only change here is that I merged three individual test functions into one class. The tests itself are copy-pasted.
|
||
assert_equal(prototype_transform(image_pil), legacy_transform(image_pil)) | ||
assert_equal(prototype_transform(image_numpy), legacy_transform(image_numpy)) | ||
def make_bounding_boxes(*args, batch_dims, **kwargs): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This was the only change I needed to make to be able to drop the whole transforms_v2_legacy_utils
module. I'm ok with that trade, since we either won't touch this file or might kill them as well (see #8013 (comment)).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok, I reverted this in 8d04cb2 since I forgot about the prototype tests. We need to port them of the legacy utils as well at some point, since these are not really legacy tests. But this for another PR.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @pmeier ,
LGTM. Do we also need to add common checks (check_transforms
, etc.) for all these test classes? Looks like we forgot to do it after TestLinearTransform
?
We should where applicable, yes. I didn't forget it, it is just not applicable to the transforms that came before this PR.
For the tests ported in this PR we should be able to add them except for |
Hey @pmeier! 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 |
Reviewed By: vmoens Differential Revision: D50789105 fbshipit-source-id: a7a1b5afe9235503311534b82497990b8d7ada3c
This is here to give us a better overview of what is left to port after #8011 and #8012. There are only a few test cases left in two files:
test/test_transforms_v2_consistency.py
: We have consistency tests against v1 as part of our regular v2 tests. Meaning, the only things left in here are transforms that are not part of v1. These are the transforms that are part of our reference scripts and became part of the v2 API. The tests for them arevision/test/test_transforms_v2_consistency.py
Line 441 in ee28bb3
vision/test/test_transforms_v2_consistency.py
Line 546 in ee28bb3
As for all the other consistency tests, these tests had the most value during the heavy development to avoid breaking consistency. Now that the API is stable and we need to keep BC anyway and the tests have been running smoothly for almost a year, their value is fairly low.
Plus, Add --use-v2 support to classification references #7724 and the sister PRs enabled the v2 transforms in the references. In the long term, we'll likely drop support for transforms v1 in the reference scripts.
My vote is out for removing them completely.
test/test_transforms_v2
: A few "transform only", i.e. no dedicated functionals, testcases remain. They have to be ported, but that should be quick.cc @vfdev-5