-
Notifications
You must be signed in to change notification settings - Fork 304
Conversation
|
||
>>> from chainer import iterators | ||
>>> | ||
>>> from chainercv.datasets import VOCDetectionDataset |
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.
VOCBBoxDataset. This is my mistake. Sorry.
|
||
|
||
def apply_to_batch(func, iterator, n_input=1, hook=None): | ||
"""Apply a function/method to an iterator of batches. |
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 is not directly related to the change proposed in this PR, but this sentence is incorrect.
We apply a function to batches and not an iterator.
Thus, Apply a function/method to batches of data from an iterator
is more precise.
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 looked up the usage of apply
on the internet.
https://pandas.pydata.org/pandas-docs/stable/generated/pandas.DataFrame.apply.html
or | ||
>>> # batch: [(in_val0, in_val1, ...)] | ||
or | ||
>>> # batch: [(in_val0, in_val1, ..., rest_val0, rest_val1, ...)] |
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.
n_input
should appear at this part of the documentation.
>>> out_vals = func(in_val0, in_val1, ...) | ||
>>> # out_vals: [out_val] | ||
or | ||
>>> out_vals0, out_vals1, ... = func(in_val0, in_val1) |
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.
in_val0, in_val1
--> in_val0, in_val1, ...
for consistency
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 these examples are not necessary.
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 would like to show the returned values should be ([out_val0], [out_val1], ...)
and not [(out_val0, out_val1, ...)]
. I feel it unclear without 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.
Ah. I see. So you wanted to emphasize that the function returns tuple of lists.
BTW, shouldn't function take list of values?
So out_vals0, out_vals1, ... = func(in_val0, in_val1)
--> out_vals0, out_vals1, ... = func(in_vals0, in_vals1)
.
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.
You are right. That's my mistake.
|
||
This function applies a function/method to an iterator of batches. | ||
It assumes that the iterator returns a batch of input data or | ||
a batch of tuples whose first elements ara input data. |
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 thought it was better to describe the internals of this function more in the beginning.
I wrote an alternative docstring. What do you think?
This function assumes that the iterator iterates over a collection of tuples that contain inputs to func
.
Additionally, the tuples may contain values that are not used by func
.
For convenience, we allow the iterator to iterate over a collection of inputs that are not tuple.
t
Conceptually, this function does two operations.
First, it splits the iterator into two iterators. The first iterator contains the inputs to func
and the second iterator contains the rest of the data from iterator
.
Second, it creates another iterator that iterates over a collection of the outputs of the func
.
Here is an illustration of the expected behavior of iterator
.
M
is the number of arguments for func
, which is defined by an argument n_input
. N
is the number of elements in a tuple from iterator
that are not used by func
. Note that N
can be 0.
>>> batch = next(iterator)
>>> # batch: [(in_val0, ..., in_val{M-1}, rest_val0, ..., rest_val{N-1})]
or
>>> # batch: [in_val]
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.
First, it splits the iterator into two iterators. The first iterator contains the inputs to func and the second iterator contains the rest of the data from iterator.
Second, it creates another iterator that iterates over a collection of the outputs of the func.
This explanation sounds this function returns three iterators. However, it is not correct.
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.
Right. That is my mistake.
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.
OK, I will try to merge your solution into docstring.
|
||
>>> in_values, out_values, rest_values = apply_to_iterator( | ||
>>> func, iterator, n_input) | ||
>>> # in_values: (iter of in_val0, ..., iter of in_val{n_input}) |
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.
n_input --> n_input-1
def apply_to_iterator(func, iterator, n_input=1, hook=None): | ||
"""Apply a function/method to batches from an iterator. | ||
|
||
This function applies a function/method to an iterator of batches. |
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 function applies a function/method to batches from an iterator of type chainer.Iterator
.
This makes clear that we are talking about Chainer's iterator.
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 is not only for chainer.Iterator
. It can work with other iterators. an iterator of type chainer.Iterator
sounds it requires some special features.
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.
OK. Then we should change args doc.
https://github.com/Hakuyume/chainercv/blob/0bd2f6d970aeaca436e6858b0e53661e364c1fd6/chainercv/utils/iterator/apply_to_iterator.py#L75
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.
Thank you for pointing out.
|
||
It assumes that the iterator iterates over a collection of tuples | ||
that contain inputs to :func:`func`. | ||
Additionally, the tuples may contain values that are not used by func. |
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.
s/by func/by the funciton
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.
LGTM except one comment.
that are not used by :func:`func`. | ||
For convenience, we allow the iterator to iterate over a collection of | ||
inputs that are not tuple. | ||
Here is an illustration of the expected behavior of the iterator. |
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 this comment is helpful for Chainer users.
Here is an ....
In short, we require iterator
to satisfy the interface of chainer.Iterator
.
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.
chainer.Iterator
requires many properties, such as batch_size
, epoch_detail
, etc..
apply_to_iterator
does not require these properties.
https://docs.chainer.org/en/stable/reference/generated/chainer.iterators.SerialIterator.html
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 chainer.Iteratror satisfies this requirement
or chainer.Iteratror satisfies this behaviour
? These are correct.
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 behaviour is same as chainer.Iterator
is also OK.
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 behaviour is the same as chainer.Iterator
This one looks good! (“the” was missing)
fix #468, fix #516
imgs, pred_values, gt_values
->in_values, out_values, rest_values
in_values
are the firstn_input
elements of the batchapply_prediction_to_iterator
->apply_to_iterator