Skip to content
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.

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

[TOPI] implement pool3d op #4478

Merged
merged 3 commits into from
Dec 12, 2019
Merged

[TOPI] implement pool3d op #4478

merged 3 commits into from
Dec 12, 2019

Conversation

optima2005
Copy link
Contributor

This is a part of attempts to implement #4009
The implemation in this PR is for max and avg 3d pooling.

@masahi

@masahi masahi self-assigned this Dec 7, 2019
src/relay/op/nn/pooling.cc Outdated Show resolved Hide resolved
@masahi
Copy link
Member

masahi commented Dec 8, 2019

@optima2005 I am traveling for another few days, will review afte I am back.

src/relay/op/nn/pooling.cc Outdated Show resolved Hide resolved
src/relay/op/nn/pooling.cc Outdated Show resolved Hide resolved
src/relay/op/nn/pooling.cc Outdated Show resolved Hide resolved
src/relay/op/nn/pooling.cc Outdated Show resolved Hide resolved
@masahi masahi merged commit 41959ed into apache:master Dec 12, 2019
@masahi
Copy link
Member

masahi commented Dec 12, 2019

thanks @optima2005 this is merged.

@optima2005 optima2005 deleted the 3dpooling branch December 13, 2019 01:29
zxy844288792 pushed a commit to zxy844288792/tvm that referenced this pull request Dec 13, 2019
* [TOPI] implement pool3d op

* use PoolInferCorrectLayout for both 2d and 3d pooling

* unify MakeMaxPool and MakeAvgPool
zxy844288792 pushed a commit to neo-ai/tvm that referenced this pull request Dec 13, 2019
* [TOPI] implement pool3d op

* use PoolInferCorrectLayout for both 2d and 3d pooling

* unify MakeMaxPool and MakeAvgPool
@deepakbabel
Copy link

Hi @optima2005 , @masahi ,
I am new to tvm community. I was trying to implement maxpool3d operator based on reference implementation from tensorflow(r=1.14) into tvm codebase. For testing the same, I have written relay and topi test cases. I am also writing test cases in test_forward.py(for tensorflow) which compares tvm output with tensorflow output. I want to know is there any specific reason why you have not added any changes in test_forward.py(comparing tvm with tf) for unit testing?? Should i ignore this file altogether?

Best Regards,
Deepak Babel

@optima2005
Copy link
Contributor Author

Hi, @deepakbabel
This PR is just to implement a 3d pooling(max&avg) operation for relay, not including the TF frontend, which is to transform the 3d pooling operation in TF model into TVM relay operation.

@deepakbabel
Copy link

Hi @optima2005 ,
Thank you for clarifying this. I want to support TF frontend in TVM for this operator(maxpool3d), so that I can compile a TF model using TVM. I am trying to ascertain changes required in TVM front-end specific files like test_forward.py and tensorflow.py file. As of now these files have support only for 2D image data format("NCHW"). Looks like there would be some small design change to include 3D("NCDHW") or nD dimension data. Do you happen to have any inputs/suggestions on the same.

@deepakbabel
Copy link

@optima2005 -
Here are the path of the files that I am referring to:
tvm/tests/python/frontend/tensorflow/test_forward.py
tvm/python/tvm/relay/frontend/tensorflow.py

Best Regards,
Deepak Babel

@optima2005
Copy link
Contributor Author

@deepakbabel, If I would implement 3D pooling TF frontend, I would create a new function by following the current 2D pooling function. And add it to the converting map for TF 3D pooling transformation. By this approach, the original 2d pooling won't be impacted and we won't handle too much conditionals in one function.

@deepakbabel
Copy link

@optima2005 : Thank you for the suggestion. I actually have done the same(seperate function for 3dpooling) in tensorflow.py file(Frontend support). Also added test cases in test_forward.py to test it. Thank you for your inputs. In test_forward.py(for Tensorflow Front-end), I could see that while creating the TVM Graph, they use "NCHW" layout by-default and currently there is no option to pass the "NCDHW" layout information in the same file. So, I guess we might have to set the layout from somewhere else(may be tensorflow.py in the new function for 3d pooling?), otherwise it will require design changes in existing test_forward.py file.
Anyways really appreciate your inputs on this.

Best Regards,
Deepak

} else if (param->padding.size() == 6) {
// (front, top, left, back, bottom, right)
pad_d = param->padding[0] + param->padding[3];
pad_h = param->padding[1] + param->padding[4];
Copy link
Contributor

Choose a reason for hiding this comment

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

pad_d and pad_h are not used. The fix is here #4738 @optima2005 @masahi

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants