Skip to content

Add vol2col functor #4462

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Merged
merged 6 commits into from
Oct 11, 2017
Merged

Add vol2col functor #4462

merged 6 commits into from
Oct 11, 2017

Conversation

chengduoZH
Copy link
Contributor

@chengduoZH chengduoZH commented Sep 28, 2017 •
edited
Loading

fix #4669

@chengduoZH chengduoZH changed the title Add vol2col functor [WIP]Add vol2col functor Sep 28, 2017
reyoung
reyoung reviewed Sep 28, 2017
View reviewed changes
paddle/operators/math/vol2col.cc Outdated
void operator()(const platform::DeviceContext& context,
const framework::Tensor& im, framework::Tensor& col,
int stride_height, int stride_width, int padding_height,
int padding_width) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

void operator()(...) const

Copy link
Contributor Author

Choose a reason for hiding this comment

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

thank you!

@chengduoZH chengduoZH force-pushed the Add_vol2col_functor branch 6 times, most recently from b7e0607 to 651ad05 Compare October 1, 2017 12:39
@chengduoZH chengduoZH force-pushed the Add_vol2col_functor branch 2 times, most recently from 275e6cd to 159ac9c Compare October 8, 2017 05:51
@chengduoZH chengduoZH changed the title [WIP]Add vol2col functor Add vol2col functor Oct 9, 2017
@chengduoZH chengduoZH force-pushed the Add_vol2col_functor branch from 4191948 to ba791f7 Compare October 9, 2017 02:39
qingqing01
qingqing01 reviewed Oct 10, 2017
View reviewed changes
paddle/operators/math/CMakeLists.txt Outdated
@@ -1,13 +1,15 @@
if(WITH_GPU)
nv_library(math_function SRCS math_function.cc math_function.cu im2col.cc im2col.cu pooling.cc pooling.cu DEPS cblas device_context operator)
nv_library(math_function SRCS math_function.cc math_function.cu im2col.cc im2col.cu vol2col.cc vol2col.cu pooling.cc pooling.cu DEPS cblas device_context operator)
Copy link
Contributor

Choose a reason for hiding this comment

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

依赖math_function的op，不一定需要依赖vol2col，所以觉得像softmax一样和math_function分开好一些

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

paddle/operators/math/vol2col.cc Outdated
((c * output_depth + d) * output_height + h) * output_width + w;
if (h_pad < 0 || h_pad >= input_height || w_pad < 0 ||
w_pad >= input_width || d_pad < 0 || d_pad >= input_depth) {
col_data[col_idx] = T(0);
Copy link
Contributor

Choose a reason for hiding this comment

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

static_cast<T>(0)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

paddle/operators/math/vol2col_test.cc Outdated
context =
new paddle::platform::CPUDeviceContext(paddle::platform::CPUPlace());
} else {
#ifndef PADDLE_ONLY_CPU
Copy link
Contributor

Choose a reason for hiding this comment

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

较新的代码去掉了PADDLE_ONLY_CPU宏定义，需要更新代码。

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

qingqing01
qingqing01 reviewed Oct 10, 2017
View reviewed changes
paddle/operators/math/vol2col_test.cc Outdated
EXPECT_EQ(out_cfo_ptr[12], 9);
EXPECT_EQ(out_cfo_ptr[13], 10);
EXPECT_EQ(out_cfo_ptr[14], 10);
EXPECT_EQ(out_cfo_ptr[15], 11);
Copy link
Contributor

Choose a reason for hiding this comment

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

这里是不是0,1,1,2,3, ... 等常量存在数组里，这里用for循环比较，代码行数少一些？

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Great!!!
Done

paddle/operators/math/vol2col.cc
int d_pad = d * stride_depth - padding_depth + d_offset;
for (int h = 0; h < output_height; ++h) {
int h_pad = h * stride_height - padding_height + h_offset;
for (int w = 0; w < output_width; ++w) {
Copy link
Contributor

Choose a reason for hiding this comment

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

这几层循环，其实觉得完全可以和 im2col实现统一成一个实现，不过这样也行吧，看着清晰一些~

@chengduoZH chengduoZH force-pushed the Add_vol2col_functor branch 2 times, most recently from 79d6689 to 0913609 Compare October 10, 2017 15:45
qingqing01
qingqing01 reviewed Oct 11, 2017
View reviewed changes
paddle/operators/math/vol2col_test.cc Outdated

TEST(math, vol2col) {
testVol2col<paddle::platform::CPUPlace>();
#ifndef PADDLE_ONLY_CPU
Copy link
Contributor

Choose a reason for hiding this comment

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

PADDLE_ONLY_CPU is still not changed.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

paddle/operators/math/CMakeLists.txt Outdated
@@ -3,11 +3,15 @@ if(WITH_GPU)
nv_test(math_function_test SRCS math_function_test.cc DEPS math_function tensor)
nv_library(softmax SRCS softmax.cc softmax.cu DEPS operator)
nv_library(cross_entropy SRCS cross_entropy.cc cross_entropy.cu DEPS operator)
nv_library(vol2col SRCS vol2col.cc vol2col.cu DEPS device_context operator)
Copy link
Contributor

Choose a reason for hiding this comment

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

Why depend on operator when compiling in this function?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

qingqing01
qingqing01 approved these changes Oct 11, 2017
View reviewed changes
Copy link
Contributor

@qingqing01 qingqing01 left a comment

Choose a reason for hiding this comment

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

LGTM.

@qingqing01 qingqing01 merged commit 62da438 into PaddlePaddle:develop Oct 11, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@reyoung reyoung reyoung left review comments

@qingqing01 qingqing01 qingqing01 approved these changes

@hedaoyuan hedaoyuan Awaiting requested review from hedaoyuan

Assignees
No one assigned
Labels
OpPorting
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Add vol2col functor.
3 participants
@chengduoZH @reyoung @qingqing01