Skip to content

Make DatasetFolder.find_classes public#3628

Merged
NicolasHug merged 8 commits intopytorch:masterfrom
alemelis:master
Apr 7, 2021
Merged

Make DatasetFolder.find_classes public#3628
NicolasHug merged 8 commits intopytorch:masterfrom
alemelis:master

Conversation

@alemelis
Copy link
Copy Markdown
Contributor

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

Closes #3627

@facebook-github-bot
Copy link
Copy Markdown
Contributor

facebook-github-bot commented Apr 1, 2021

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
Copy Markdown
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

@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
Copy Markdown
Member

@NicolasHug NicolasHug Apr 1, 2021

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
Copy Markdown
Member

@NicolasHug NicolasHug Apr 1, 2021

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
Copy Markdown
Contributor Author

@alemelis alemelis Apr 1, 2021

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
Copy Markdown
Member

@NicolasHug NicolasHug Apr 1, 2021

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
Copy Markdown
Contributor

facebook-github-bot commented Apr 1, 2021

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
Copy Markdown
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
Copy Markdown
Member

NicolasHug commented Apr 2, 2021

@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
Copy Markdown
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
Copy Markdown
Member

NicolasHug commented Apr 7, 2021

Thanks @alemelis !

@fmassa
Copy link
Copy Markdown
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