Skip to content

Make DatasetFolder.find_classes public #3628

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 8 commits into from
Apr 7, 2021
Merged

Make DatasetFolder.find_classes public #3628

merged 8 commits into from
Apr 7, 2021

Conversation

alemelis
Copy link
Contributor

@alemelis alemelis commented Apr 1, 2021 •
edited by NicolasHug
Loading

Closes #3627

@facebook-github-bot
Copy link

Hi @alemelis!

Thank you for your pull request and welcome to our community.

Action Required

In order to merge any pull request (code, docs, etc.), we require contributors to sign our Contributor License Agreement, and we don't seem to have one on file for you.

Process

In order for us to review and merge your suggested changes, please sign at https://code.facebook.com/cla. If you are contributing on behalf of someone else (eg your employer), the individual CLA may not be sufficient and your employer may need to sign the corporate CLA.

Once the CLA is signed, our tooling will perform checks and validations. Afterwards, the pull request will be tagged with CLA signed. The tagging process may take up to 1 hour after signing. Please give it that time before contacting us about it.

If you have received this in error or have any questions, please contact us at [email protected]. Thanks!

NicolasHug
NicolasHug reviewed Apr 1, 2021
View reviewed changes
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.

Thanks for the PR @alemelis

torchvision/datasets/folder.py Outdated
@@ -203,7 +203,7 @@ def make_dataset(
return make_dataset(directory, class_to_idx, extensions=extensions, is_valid_file=is_valid_file)

@staticmethod
def _find_classes(dir: str) -> Tuple[List[str], Dict[str, int]]:
def find_classes(dir: str) -> Tuple[List[str], Dict[str, int]]:
Copy link
Member

Choose a reason for hiding this comment

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

Let's make this an instance method too and remove the @staticmethod following #3496 (review)

Copy link
Member

Choose a reason for hiding this comment

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

Also, we'll need docs now :)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yep instance method makes sense

there's already a quite detailed doctstring in find_classes() function, shall I move it to this method instead?

Copy link
Member

Choose a reason for hiding this comment

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

I would link to find_classes() and write something like

Same as :func:find_classes. This method can be overridden to only consider a subset of classes, or to adapt to a different dataset directory structure.

@facebook-github-bot
Copy link

Thank you for signing our Contributor License Agreement. We can now accept your code for this (and any) Facebook open source project. Thanks!

NicolasHug
NicolasHug approved these changes Apr 2, 2021
View reviewed changes
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.

Nit but LGTM, thanks!

@NicolasHug
Copy link
Member

@alemelis before we merge would you mind looking into the linting and test issues?

@alemelis
fix python_lint
216f3a0 
./torchvision/datasets/folder.py:207:1: W293 blank line contains whitespace
./torchvision/datasets/folder.py:208:55: W291 trailing whitespace
@alemelis
Copy link
Contributor Author

alemelis commented Apr 2, 2021

@alemelis before we merge would you mind looking into the linting and test issues?

torchhub_test is failing with

ValueError: Cannot find master in https://github.com/pytorch/vision. If it's a commit from a forked repo, please call hub.load() with forked repo directly.

Since I'm pushing from a fork, is there an additional step I should follow to fix this?

@NicolasHug NicolasHug mentioned this pull request Apr 4, 2021
Merged
@NicolasHug NicolasHug changed the title Make find_classes public Make DatasetFolder.find_classes public Apr 7, 2021
@NicolasHug NicolasHug merged commit f9af70a into pytorch:master Apr 7, 2021
@NicolasHug
Copy link
Member

Thanks @alemelis !

@fmassa
Copy link
Member

fmassa commented Apr 7, 2021

Thanks @alemelis for the PR and @NicolasHug for the review!

facebook-github-bot pushed a commit that referenced this pull request Apr 13, 2021
@fmassa @facebook-github-bot
[fbsync] Make DatasetFolder.find_classes public (#3628)
c1ec02e 
Summary: Co-authored-by: Nicolas Hug <[email protected]>

Reviewed By: NicolasHug

Differential Revision: D27706955

fbshipit-source-id: da038e3b92aa47a720b45f3b6ea4a48976e15ed2
@NicolasHug NicolasHug mentioned this pull request Apr 25, 2021
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@NicolasHug NicolasHug NicolasHug approved these changes

Assignees
No one assigned
Labels
cla signed enhancement module: datasets
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Select subset of classes to sample from
5 participants
@alemelis @facebook-github-bot @NicolasHug @fmassa @datumbox