Skip to content
This repository was archived by the owner on Jul 2, 2021. It is now read-only.

Add apply_prediction_to_iterator #229

Merged
merged 67 commits into from
Jun 1, 2017

Conversation

Hakuyume
Copy link
Member

@Hakuyume Hakuyume commented May 31, 2017

merge after #227

@Hakuyume Hakuyume changed the title Add apply_prediction_to_iterator [WIP] Add apply_prediction_to_iterator May 31, 2017
@yuyu2172
Copy link
Member

yuyu2172 commented May 31, 2017

This is a great idea!
This function can be applied not only to semantic segmentation and object detection, but also to image retrieval tasks (and many others!).

* :obj:`gt_values`: A tuple of iterators. Each iterator \
returns a corresponding ground truth value. If the input \
iterator does not give any ground truth values, this tuple \
will be empty.
Copy link
Member

@yuyu2172 yuyu2172 Jun 1, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Perhaps, we can add an example for gt_values as well.

:func:`predict` as an argument.
The rests are treated as ground truth values.
hook: A callable which is called after each iteration.
:obj:`imgs`, :obj:`pred_labels` and :obj:`gt_values` are passed as
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think pred_labels is wrong

>>> pred_vals0, pred_vals1 = predict(imgs)
>>> # pred_vals0: [pred_val0]
>>> # pred_vals1: [pred_val1]

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A simple working code example would help.

and :obj:`next(pred_values[1])` will be \
:obj:`pred_val0` and :obj:`pred_val1`.
* :obj:`gt_values`: A tuple of iterators. Each iterator \
returns a corresponding ground truth value.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You forgot \.

>>> # pred_vals0: [pred_val0]
>>> # pred_vals1: [pred_val1]

Here is an exmple which applies a pretrained Faster R-CNN to
Copy link
Member

@yuyu2172 yuyu2172 Jun 1, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think you need a comma before which because this is a nonrestrictive phrase.

A nonrestrictive phrase adds a little bit of extra (but not essential) information about a noun phrase that you’ve already mentioned in your sentence.

https://www.grammarly.com/blog/comma-before-which/

Also, I have read that that is preferred in a restrictive phrase case.
Perhaps, we can change which used in Args to that.

Although some writers use "which" to introduce a restrictive clause, the traditional practice is to use "that" to introduce a restrictive clause and "which" to introduce a nonrestrictive clause.
http://www.kentlaw.edu/academics/lrw/grinker/LwtaClauses__Restrictive_and_Nonrest.htm

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think whichs used in Args are restrictive.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If its restrictive, don't we not use commas?

Also, I think , which should be used for this line, and that (no comma) for Args.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry, I was misunderstanding.

>>> dataset = VOCDetectionDataset(year='2007', split='test')
>>> # next(iterator) -> [(img, gt_bbox, gt_label)]
>>> iterator = iterators.SerialIterator(
... dataset, 32, repeat=False, shuffle=False)
Copy link
Member

@yuyu2172 yuyu2172 Jun 1, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How about batchsize 2?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it is better to set batchsize more than 1 because we can show apply_prediction_to_iterator flattens batches.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry. I changed my initial comment. How about 2?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think 2 is OK. I fixed.

@yuyu2172
Copy link
Member

yuyu2172 commented Jun 1, 2017

Except for which and that, LGTM

@yuyu2172 yuyu2172 merged commit 02f6d33 into chainer:master Jun 1, 2017
@Hakuyume Hakuyume deleted the add-apply-prediction branch June 1, 2017 01:54
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants