-
Notifications
You must be signed in to change notification settings - Fork 7k
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
[docs] initial docstring for makedataset #2879
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @bjuncek ! I left few comments but it's a nitpicking.
torchvision/datasets/folder.py
Outdated
is_valid_file should not be passed. Defaults to None. | ||
|
||
Raises: | ||
ValueError: In case ``extensions`` and ``is_valid_file`` is None. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Technically, this is raised if both_none
or both_something
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
addressed in a following commit
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hey @bjuncek thanks for the PR! YOu have included a lot of unrelated formatting changes. Could you revert them?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@bjuncek thanks for the update. I have a small comment about how you write arg type (as typing annotation). I'm not sure about its rendering in the docs. Could you please check that ?
torchvision/datasets/folder.py
Outdated
Args: | ||
directory (str): root dataset directory | ||
class_to_idx (Dict[str, int]): dictionary mapping class name to class index | ||
extensions (Optional[Tuple[str, ...]], optional): A list of allowed extensions. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Optional is repeated twice now (here and below). I'm not sure if we started to add arg type as typing annotations... @pmeier what do you think ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we should stick to one form. Either
extensions (optional): A list of allowed extensions.
or
extensions (Optional[Tuple[str, ...]]): Allowed extensions.
I prefer the second version especially because the type hints can be auto generated from the annotations. That being said we need a sphinx
extension for that and thus we should stick to the first version for now.
I've just realized that the type annotation and the docstring contradict each other. Should this be a list
or a tuple
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed.
the latter type was auto-generated, sorry about that. Pushed the changes that hopefully resolve this :)
Please let me know if this is OK to merge :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks!
* initial docstring * Revert "initial docstring" This reverts commit 2bf68ca. * revert the formatting changes * clear up per Victor's comment * Addressing PR comments Co-authored-by: Bruno Korbar <korbar@vggdev9.vggdev.cluster>
* initial docstring * Revert "initial docstring" This reverts commit 2bf68ca. * revert the formatting changes * clear up per Victor's comment * Addressing PR comments Co-authored-by: Bruno Korbar <korbar@vggdev9.vggdev.cluster>
per issue
#2841
@vfdev-5 can you please check out that the wording is ok?