-
Notifications
You must be signed in to change notification settings - Fork 305
Conversation
I like how you kept the design simple. I found two problems. import time
import numpy as np
x = np.arange(1000000) # 1M
start = time.time()
out = list()
for i in range(len(x)):
out.append(x[i])
print(time.time() - start) # 0.1742 s Also, there is another person making similar feature in Chainer project. |
Yes. If annotations contain some large data, my API is not efficient.
What do you mean?
I see. That PR is more general than mine. If that PR is merged, we don't need to add |
If a dataset has an array |
I see. That makes sense. However, if users use only a subset of the dataset and labels are stored under multiple files, returning a list is not efficient. For example, If I want to filter the first 100 examples of VOCBboxDataset, loading |
I see. Since the purpose of this PR conflicts with #3252 and this is not unique to Vision, I think the design should be discussed with Chainer team. |
Since #3252 does not seem to solve the efficiency problem that we have been discussing, I changed my mind and think that this dataset is appropriate for ChainerCV.
|
I think it is reasonable to keep our own mix-in. The style of chainer/chainer#3252 is not optimized to ChainerCV. If we use chainer/chainer#3252, we have to implement 5 methods for each dataset.
A simple solution is excluding def get_lightweight_annotation(self, i):
...
return label
def get_image(self, i):
...
img = read_image(...)
return img
def get_example(self, i): # the default implementation returns (img, label)
...
mask = read_image(...)
...
label = self.get_lightweight_annotation(i)
img = self.get_image(i)
return img, mask, label This workaround can be used for |
I have two problems. Second, it became more complicated than what it was. How about setting an attribute that configures BTW, shouldn't |
In that case,
Yes. That is a problem. |
What do you mean? |
I'm trying to implement better APIs. |
How about this API? I have implemented class MyDataset(PickableDataset):
def __init__(self):
super().__init__()
self.data_names = ('img', 'bbox', 'label', 'mask')
self.add_getter('img', self.get_image)
self.add_getter('mask', self.get_mask)
self.add_getter(('bbox', 'label'), self.get_bbox_label)
def get_image(self, i):
print('get_image')
return 'img_{:d}'.format(i)
def get_mask(self, i):
print('get_mask')
return 'mask_{:d}'.format(i)
def get_bbox_label(self, i):
print('get_bbox_label')
return 'bbox_{:d}'.format(i), 'label_{:d}'.format(i)
dataset = MyDataset()
print(dataset[0])
# get_image
# get_bbox_label
# get_mask
# ('img_0', 'bbox_0', 'label_0', 'mask_0')
picked_dataset = dataset.pick('label')
print(picked_dataset[1])
# get_bbox_label
# label_1
picked_dataset = dataset.pick('img', 'label', 'bbox')
print(picked_dataset[2])
# get_image
# get_bbox_label
# ('img_2', 'label_2', 'bbox_2') |
I am not sure whether we should introduce a new abstraction or remain with the minimal abstraction ( In my opinion, by the nature that the class is intended to be extended by users, this has to be perfect or should not be used. I think there is still to be improved. |
I don't think providing partial access only for
I agree. I would like to make this class perfect as much as possible.
Good idea. I'll fix it. |
Ideally, it is beneficial to have a dataset abstraction that never changes its functionality by modifying it. On top of that, we have another demand that subset of dataset annotations should be easily and efficiently accessible. This is the problem that we have been discussing from the beginning. With that in mind, I suggest to introduce a new dataset class possibly different from If we take this strategy, chainer/chainer#3343 and chainer/chainer#3252 should be replaced.
We can add |
Will you add |
I see what you want to say. If all of them are going to be supported, we need to take more general approach with base dataset class. |
Can you add at least one code example to the doc string? |
@Hakuyume |
class ConcatenatedDataset(SliceableDataset): | ||
"""A sliceable version of :class:`chainer.datasets.ConcatenatedDataset`. | ||
|
||
Hew is an example. |
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.
Here
Args: | ||
datasets: The underlying datasets. | ||
Each dataset should inherit | ||
:class:~chainer.datasets.sliceable.Sliceabledataset`. |
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.
no period
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.
Not chainer.datasets
, but chainercv.*
|
||
|
||
class GetterDataset(SliceableDataset): | ||
"""A sliceable dataset class that defined by getters. |
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.
that is defined with
This ia a dataset class that supports slicing. | ||
A dataset class inheriting this class should implement | ||
three methods: :meth:`__len__`, :meth:`keys`, and | ||
:meth:`get_example_by_keys`. |
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.
Should we recommend users to use GetterDataset
?
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 mean the relationship between SlicableDataset
and GetterDataset
should be clear.
From users perspective,
they would first come and read SlicableDataset
. Since this is not intended to be directly touched by users, we should guide them to GetterDataset
.
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.
Nice suggestion. I agree with you.
Note that it reuqires :obj:`keys` to determine the names of returned | ||
values. | ||
|
||
Hew is an example. |
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.
Here
class TupleDataset(SliceableDataset): | ||
"""A sliceable version of :class:`chainer.datasets.TupleDataset`. | ||
|
||
Hew is an example. |
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.
Here
|
||
Args: | ||
datasets: The underlying datasets. | ||
Following datasets are acceptable. |
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.
The following datasets
|
||
>>> class SliceableLabeledImageDataset(GetterDataset): | ||
>>> def __init__(self, pairs, root='.'): | ||
>>> super().__init__() |
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.
Python 2 would not work.
>>> # get a subset with label = 0, 1, 2 | ||
>>> # no images are loaded | ||
>>> indices = [i for i, label in | ||
>>> enumerate(dataset.slice[:, 'label']) if label in {0, 1, 2}] |
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.
indentation is wiered
def slice(self): | ||
return SliceHelper(self) | ||
|
||
def __iter__(self): |
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.
Do we need this?
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.
it is not necessary for the sliceable functionality. I added this for convenience.
In the case of calculating statistics of labels, we can write
collections.Counter(dataset[:, 'label'])
Without __iter__
, we have to write
collections.Counter(dataset[:, 'label'][:]) # temporary list creation (not good for both speed and memory)
# or
collections.Counter(dataset[:, 'label'][i] for i in range(len(dataset))) # lengthy
docs/source/tutorial/sliceable.rst
Outdated
img, bbox, label = dataset[0] # get_image(0) and get_annotation(0) | ||
|
||
view = dataset.slice[:, 'label'] | ||
label = view[1] # get_annotation(0) |
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.
get_annotaion(1)
docs/source/tutorial/sliceable.rst
Outdated
return bbox, label | ||
|
||
|
||
dataset = SampleBboxdataset() |
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.
typo
docs/source/tutorial/sliceable.rst
Outdated
from chainercv.datasets import VOCBboxDataset | ||
dataset = VOCBboxDataset() | ||
|
||
# the view of label of the first 100 examples |
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.
the view of the labels of the first 100 examples
docs/source/tutorial/sliceable.rst
Outdated
# the view of first 100 examples | ||
view = dataset.slice[:100] | ||
|
||
# the view of last 100 examples |
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.
the last
docs/source/tutorial/sliceable.rst
Outdated
from chainercv.datasets import VOCBboxDataset | ||
dataset = VOCBboxDataset() | ||
|
||
# the view of first 100 examples |
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.
the first
docs/source/tutorial/sliceable.rst
Outdated
where :method:`DatasetMixin.__getitem__` conducts :method:`get_example` for all required examples. | ||
Users can write efficient code by this view. | ||
|
||
This example counts the number of images that contain dogs. |
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.
... contain dogs without loading any images.
docs/source/tutorial/sliceable.rst
Outdated
We assume that readers have a basic understanding of Chainer dataset (e.g. understand :class:`chainer.dataset.DatasetMixin`). | ||
|
||
In ChainerCV, we introduce `sliceable` feature to datasets. | ||
SliceableT datasets support :method:`slice` that returns a sub view of the dataset. |
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.
typo
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.
a sub view --> a view
docs/source/tutorial/sliceable.rst
Outdated
In ChainerCV, we introduce `sliceable` feature to datasets. | ||
SliceableT datasets support :method:`slice` that returns a sub view of the dataset. | ||
|
||
This example that shows the basic usage. |
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.
This example shows the basic usage.
docs/source/tutorial/sliceable.rst
Outdated
img, bbox, label = dataset[0] # get_image(0) and get_annotation(0) | ||
|
||
view = dataset.slice[:, 'label'] | ||
label = view[1] # get_annotation(0) |
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.
get_annotation(1)
class GetterDataset(SliceableDataset): | ||
"""A sliceable dataset class that is defined with getters. | ||
|
||
This ia a dataset class with getters. |
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.
This ia --> This is a
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.
How about adding a comment that lets users know about the tutorial?
Please refer to the tutorial for more detailed explanation.
The following datasets are acceptable. | ||
|
||
* An inheritance of \ | ||
:class:~chainer.datasets.sliceable.SliceableDataset`. |
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.
does not compile properly
Args: | ||
datasets: The underlying datasets. | ||
Each dataset should inherit | ||
:class:~chainercv.chainer_experimental.datasets.sliceable.Sliceabledataset` |
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.
does not compile properly
The result of benchmark in the tutorial
|
related to #453
This PR adds
AnnotatedImageDatasetMixin
.This mix-in requires these methods.
__len__
: returns the length of datasetget_image
: takes an index and returns an imageget_annotation
: takes an index and returns annotation(s)This mix-in provides these attribute/method.
__getitem__
: takes an index/slice and returns example(s). An example is a tuple of image and annotations. Ifget_annotation
returns(anno0, anno1)
, an example will be(img, anno0, anno1)
.annotations
: a dataset without images. Ifget_annotation
returns(anno0, anno1)
, an example of this dataset will be(anno0, anno1)
.