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

remove videos from test for DatasetFolder #7216

Merged
merged 1 commit into from
Feb 10, 2023

Conversation

pmeier
Copy link
Contributor

@pmeier pmeier commented Feb 10, 2023

The test mixed images and videos and this made it an outlier for two reasons:

  1. It inherited from the image test case, but this meant it missed out on

    REQUIRED_PACKAGES = ("av",)

    coming from the video test case. This was not an issue until Add Python 3.11 Linux CPU Unittesting #7155 (comment).

  2. Since the test needed to handle images and videos at the same time, we didn't bother to write an actual loader, but just returned the path back:

    vision/test/test_datasets.py

    Lines 1550 to 1551 in c974742

    def dataset_args(self, tmpdir, config):
    return tmpdir, lambda x: x

    Again, not an issue until Compatibility layer between stable datasets and prototype transforms #6663 (comment).

Both issues individually could be fixed, by adding the missing config parameter or by special casing it for the newly added tests. However, I don't think we gain much here by checking video and images.

Thus, this PR removes the video stuff from the test case and just focuses on the images. Note that in contrast to the test case for ImageFolder, we still test multiple extensions and make sure that we didn't pick up anything unrelated.

@pmeier pmeier changed the title Test datasetfolder remove videos from test for DatasetFolder Feb 10, 2023
Copy link
Member

@NicolasHug NicolasHug left a comment

Choose a reason for hiding this comment

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

So basically the test now checks that DatasetFolder focuses on the proper file extensions, and that the right loader is used? Sounds like this is enough for videos as well, even though we're not testing those directly anymore - LGTM!

@pmeier pmeier merged commit 17088a6 into pytorch:main Feb 10, 2023
@pmeier pmeier deleted the test-datasetfolder branch February 10, 2023 13:58
facebook-github-bot pushed a commit that referenced this pull request Mar 28, 2023
Reviewed By: vmoens

Differential Revision: D44416280

fbshipit-source-id: 9fc6193628125e8ae2bdc3cddfbaec082d04c396
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