Skip to content

Conversation

23pointsNorth
Copy link
Contributor

Current behaviour:
torchvision.datasets.SVHN(root=' /some/dir/here', download=True) downloads the train_32x32.mat directly to root.

Issue:
It doesn't create an SVHN folder where the mat is stored, as the description of the root variable is.

Solution:
With this fix, the mat file will be downloaded in a root+'SVHN' folder and will be read from there. This way, the data is not placed in root without any label what the dataset is and is in a corresponding folder..

Current behaviour:
`torchvision.datasets.SVHN(root=' /some/dir/here', download=True)` downloads the `train_32x32.mat` directly to root. 

Issue:
It doesn't create an SVHN folder where the mat is stored, as the description of the `root` variable is.

Solution:
With this fix, the `mat` file will be downloaded in a root+'SVHN' folder and will be read from there. This way, the data is not placed in root without any label what the dataset is and is in a corresponding folder..
@23pointsNorth
Copy link
Contributor Author

Can I have some information about the failed tests? Looking for details requires authorizing access to the bot.

@pmeier
Copy link
Collaborator

pmeier commented Aug 2, 2020

HI @23pointsNorth thanks for the PR!

While your fix aligns the functionality with the documentation, it breaks BC. I think it is more reasonable to change the documentation to actually state what is happening. Thoughts?

Looking for details requires authorizing access to the bot.

I've also struggled with this in the past. Strangely enough, you only have to be logged in at CircleCI to see the results. In the past I've just logged in with my GitHub and visited the link again to see the run.

Can I have some information about the failed tests?

The following test fails, since it tests the old behavior:

@mock.patch('torchvision.datasets.SVHN._check_integrity')
@unittest.skipIf(not HAS_SCIPY, "scipy unavailable")
def test_svhn(self, mock_check):
mock_check.return_value = True
with svhn_root() as root:
dataset = torchvision.datasets.SVHN(root, split="train")
self.generic_classification_dataset_test(dataset, num_images=2)
dataset = torchvision.datasets.SVHN(root, split="test")
self.generic_classification_dataset_test(dataset, num_images=2)
dataset = torchvision.datasets.SVHN(root, split="extra")
self.generic_classification_dataset_test(dataset, num_images=2)

@fmassa
Copy link
Member

fmassa commented Aug 3, 2020

I agree with @pmeier here, I think it would be preferable to change the documentation so that we keep backwards-compatibility.

@yassineAlouini
Copy link
Contributor

I think the best thing to do here is to update the documentation as suggested by @pmeier. Should we close this PR in the time being or push the documentation update here? 🤔

@pmeier
Copy link
Collaborator

pmeier commented May 2, 2022

I would close the PR and go for the documentation change. Would you be up for that @yassineAlouini?

@yassineAlouini
Copy link
Contributor

I would close the PR and go for the documentation change. Would you be up for that @yassineAlouini?

Yes, that works for me. 👌

@23pointsNorth 23pointsNorth mentioned this pull request May 2, 2022
@23pointsNorth
Copy link
Contributor Author

Closing this PR and adding just the docs change in #5930.

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.

5 participants