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

Add PSROIPooling Function #545

Merged
merged 4 commits into from
Mar 29, 2018
Merged

Add PSROIPooling Function #545

merged 4 commits into from
Mar 29, 2018

Conversation

knorth55
Copy link
Contributor

@knorth55 knorth55 commented Mar 27, 2018

Related to #538

  • add psroi_pooling_2d
  • add test for psroi_pooling_2d
  • add doc for psroi_pooling_2d
  • add test to check CPU and GPU output is same.

@knorth55 knorth55 self-assigned this Mar 27, 2018
@knorth55 knorth55 requested a review from yuyu2172 March 27, 2018 05:36
Args:
x (~chainer.Variable): Input variable. The shape is expected to be
4 dimentional: (n: batch, c: channel, h, height, w: width).
rois (~chainer.Variable): Input roi variable. The shape is expected to
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 it is not necessary to make the API of this function to follow chainer.functions.roi_pooling_2d.
Rather, it would be better to follow the preferred interface of ChainerCV.

Can you change the second input to rois and roi_indices as defined here? (bbox is yx order)

            rois (array): A bounding box array containing coordinates of
                proposal boxes.  This is a concatenation of bounding box
                arrays from multiple images in the batch.
                Its shape is :math:`(R', 4)`. Given :math:`R_i` proposed
                RoIs from the :math:`i` th image,
                :math:`R' = \\sum _{i=1} ^ N R_i`.
            roi_indices (array): An array containing indices of images to
                which bounding boxes correspond to. Its shape is :math:`(R',)`.

https://github.com/chainer/chainercv/blob/master/chainercv/links/model/faster_rcnn/faster_rcnn_vgg.py#L201

Copy link
Member

Choose a reason for hiding this comment

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

This would require some change to kernels.

def test_backward_gpu(self):
self.check_backward(cuda.to_gpu(self.x), cuda.to_gpu(self.rois),
cuda.to_gpu(self.gy))

Copy link
Member

Choose a reason for hiding this comment

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

"""Position Sensitive Region of Interest (ROI) pooling function.

This function computes position sensitive average of input spatial patch
with the given region of interest. Each ROI are splitted into
Copy link
Member

Choose a reason for hiding this comment

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

region of interest --> region of interests

Each ROI are --> Each ROI is

This function computes position sensitive average of input spatial patch
with the given region of interest. Each ROI are splitted into
:math:`(group\_size, group\_size)` regions, and position sensitive values
in each region is inferenced.
Copy link
Member

Choose a reason for hiding this comment

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

infrenced --> computed


bottom_data, bottom_rois = inputs
channels, height, width = bottom_data.shape[1:]
n_rois = bottom_rois.shape[0]
Copy link
Member

Choose a reason for hiding this comment

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

Could you follow ChainerCV's naming convention? (n_{Singular} instead of n_{Plural})
n_rois --> n_roi

roi_width = max(xmax - xmin, 0.1)
roi_height = max(ymax - ymin, 0.1)

strided = 1. * channels / self.outd
Copy link
Member

Choose a reason for hiding this comment

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

Could you remove 1. and add from __future__ import division?

roi_width = max(xmax - xmin, 0.1)
roi_height = max(ymax - ymin, 0.1)

strideh = 1. * roi_height / self.outh
Copy link
Member

Choose a reason for hiding this comment

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

ditto

for i_roi in six.moves.range(n_rois):
idx, xmin, ymin, xmax, ymax = bottom_rois[i_roi]
idx = int(idx)
xmin = round(xmin * self.spatial_scale)
Copy link
Member

Choose a reason for hiding this comment

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

The variables are defined in xy, wh and yx, hw orders.
This is inconsistent. Could you reorder them to yx, hw?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

can it be different from roi_pooling_2d in chainer master branch?

Copy link
Member

Choose a reason for hiding this comment

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

@@ -0,0 +1,331 @@
# Cuda Kernel Original work by Haozhi Qi (@Oh233)
Copy link
Member

Choose a reason for hiding this comment

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

Mention license

@knorth55
Copy link
Contributor Author

@yuyu2172 I'm sorry that I rebase the commits again.
I follow your review and update.
Other thing is that I updated variables such as outh -> out_h and sliceh -> slice_h.
Also, I renamed outd -> out_c.

@knorth55 knorth55 changed the title Add PSROIPooling Function [WIP] Add PSROIPooling Function Mar 27, 2018
@knorth55 knorth55 force-pushed the psroipooling branch 2 times, most recently from aa2c88b to e63e5e9 Compare March 28, 2018 15:16
@knorth55 knorth55 changed the title [WIP] Add PSROIPooling Function Add PSROIPooling Function Mar 28, 2018

@attr.gpu
@condition.retry(3)
def test_consistency_with_gpu(self):
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I check that calculation output of GPU and CPU are same in this test.

@knorth55 knorth55 force-pushed the psroipooling branch 4 times, most recently from 595e70e to 0d75610 Compare March 28, 2018 16:32
x_min = round(x_min * self.spatial_scale)
y_max = round(y_max * self.spatial_scale)
x_max = round(x_max * self.spatial_scale)
roi_width = max(x_max - x_min, 0.1)
Copy link
Member

Choose a reason for hiding this comment

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

Order of the two lines are opposite.

roi_height = max(y_max - y_min, 0.1)
roi_width = max(x_max - x_min, 0.1)

'float32 top_data',
'''
// pos in output filter
int pw = i % pooled_width;
Copy link
Member

Choose a reason for hiding this comment

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

ditto

round(bottom_rois[n * 4 + 3])) * spatial_scale;

// Force too small ROIs to be 1x1
float roi_width = max(roi_end_w - roi_start_w, 0.1); // avoid 0
Copy link
Member

Choose a reason for hiding this comment

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

ditto

bool is_empty = (hend <= hstart) || (wend <= wstart);

// Compute c at bottom
int gw = floor(
Copy link
Member

Choose a reason for hiding this comment

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

ditto

static_cast<float>(pw) * group_size / pooled_width);
int gh = floor(
static_cast<float>(ph) * group_size / pooled_height);
gw = min(max(gw, 0), group_size - 1);
Copy link
Member

Choose a reason for hiding this comment

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

ditto

static_cast<float>(pw) * bin_size_w + roi_start_w);
int hstart = floor(
static_cast<float>(ph) * bin_size_h + roi_start_h);
int wend = ceil(
Copy link
Member

Choose a reason for hiding this comment

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

ditto

static_cast<float>(ph + 1.0) * bin_size_h + roi_start_h);

// Add roi offsets and clip to input boundaries
wstart = min(max(wstart, 0), width);
Copy link
Member

Choose a reason for hiding this comment

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

ditto

// Add roi offsets and clip to input boundaries
wstart = min(max(wstart, 0), width);
hstart = min(max(hstart, 0), height);
wend = min(max(wend, 0), width);
Copy link
Member

Choose a reason for hiding this comment

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

ditto

bool is_empty = (hend <= hstart) || (wend <= wstart);

// Compute c at bottom
int gw = floor(
Copy link
Member

Choose a reason for hiding this comment

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

ditto

static_cast<float>(pw) * group_size / pooled_width);
int gh = floor(
static_cast<float>(ph) * group_size / pooled_height);
gw = min(max(gw, 0), group_size - 1);
Copy link
Member

Choose a reason for hiding this comment

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

ditto

@yuyu2172 yuyu2172 merged commit 50cf990 into chainer:master Mar 29, 2018
@yuyu2172 yuyu2172 added this to the v0.9 milestone Mar 29, 2018
@knorth55 knorth55 deleted the psroipooling branch March 29, 2018 15:59
@yuyu2172 yuyu2172 mentioned this pull request May 28, 2018
21 tasks
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants