Minor changes to prototype datasets #5282
Conversation
💊 CI failures summary and remediationsAs of commit 0a2563e (more details on the Dr. CI page): 💚 💚 Looks good so far! There are no failures yet. 💚 💚 This comment was automatically generated by Dr. CI (expand for details).Please report bugs/suggestions to the (internal) Dr. CI Users group. |
| @@ -23,8 +23,7 @@ def register(dataset: Dataset) -> None: | |||
| register(obj()) | |||
|
|
|||
|
|
|||
| # This is exposed as 'list', but we avoid that here to not shadow the built-in 'list' | |||
| def _list() -> List[str]: | |||
| def list_names() -> List[str]: | |||
There was a problem hiding this comment.
I went for list_names since it corresponds to the name parameter of the load() function. Alternatives I could think of were more verbose (e.g. list_available_datasets()) but I'm happy to hear others thoughts
There was a problem hiding this comment.
To recap: list is a fitting name and this works fine if you do something like
from torchvision.prototype import datasets
datasets.list()but you cannot (or better should not) do
from torchvision.prototype.datasets import listSince we don't want to enforce a specific style on the user, we should use a name that works in both cases.
There was a problem hiding this comment.
TensorFlow datasets uses list_builders probably for the same reason. That would correspond to lists_datasets for us, which I like a little better than list_names.
| @@ -23,8 +23,7 @@ def register(dataset: Dataset) -> None: | |||
| register(obj()) | |||
|
|
|||
|
|
|||
| # This is exposed as 'list', but we avoid that here to not shadow the built-in 'list' | |||
| def _list() -> List[str]: | |||
| def list_names() -> List[str]: | |||
There was a problem hiding this comment.
To recap: list is a fitting name and this works fine if you do something like
from torchvision.prototype import datasets
datasets.list()but you cannot (or better should not) do
from torchvision.prototype.datasets import listSince we don't want to enforce a specific style on the user, we should use a name that works in both cases.
| @@ -23,8 +23,7 @@ def register(dataset: Dataset) -> None: | |||
| register(obj()) | |||
|
|
|||
|
|
|||
| # This is exposed as 'list', but we avoid that here to not shadow the built-in 'list' | |||
| def _list() -> List[str]: | |||
| def list_names() -> List[str]: | |||
There was a problem hiding this comment.
TensorFlow datasets uses list_builders probably for the same reason. That would correspond to lists_datasets for us, which I like a little better than list_names.
Co-authored-by: Philip Meier <[email protected]>
… datasets_comments
Summary: * Some Qs * Some modifications * don't need _loader in __init__ * list_names -> list_datasets * Update torchvision/prototype/datasets/utils/_resource.py * Remove unsued import * fix tests * Some missing renames Reviewed By: kazhang Differential Revision: D33927488 fbshipit-source-id: f30c15e79b8a5c41b56845adfc970cc40130b92c Co-authored-by: Philip Meier <[email protected]> Co-authored-by: Philip Meier <[email protected]>
Some minor changes we discussed earlier with @pmeier