Skip to content

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 requested a review from NicolasHug February 10, 2023 13:14
@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