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

Support non-square padding for Convolution operator #1107

Closed
artemrakhov-glow opened this issue Jun 6, 2018 · 3 comments
Closed

Support non-square padding for Convolution operator #1107

artemrakhov-glow opened this issue Jun 6, 2018 · 3 comments
Assignees

Comments

@artemrakhov-glow
Copy link
Contributor

artemrakhov-glow commented Jun 6, 2018

Convolution is the most important operator for Computer Vision NNs.

The simplest implementation (our Interpreter backend) could be found here:

void Interpreter::fwdConvolutionInst_FloatImpl(Value *inV, Value *outV,

It has parameters, such as group, pad, stride, filterSize.

Convolution is 2D operation (it always works with 2D image and 2D filter, and produces 2D result). Right now all our Convolution implementations assume that pad is the same for both dimensions (image height and width). But for some models (very rarely) it's not true. In general case, there could be 4 different pads: left side, right side, top side, bottom side. We need to change "pad" to be a vector instead of a single number (for ConvolutionNode and ConvolutionInstr). And change all the implementations: Interpreter (float and quantized), CPU (float and quantized). Later, we also need to support uneven paddings in ConvolutionGradNode (which is backward pass for Convolution).

Note, that our Convolution for CPU backend is highly optimized, and in theory complicating its code may lead to losing performance. In practice, I highly doubt that this could happen, but worth to check performance after the change just to be sure. One can run tests/images/run.sh -cpu -iterations=100 with and without the change, and make sure that average performance numbers are within 1%.

Here is ONNX specification of Conv operator:
https://github.com/onnx/onnx/blob/master/docs/Operators.md#conv
One can notice that not only padding is represented as array there, but also stride and filterSize are represented as array. Changing these is lower priority, and there's no immediate need. Just good thing to have.

Another direction of future work: support non-square padding in PoolMax and PoolAvg operators.

@beicy beicy self-assigned this Jun 7, 2018
@rdzhabarov
Copy link
Contributor

@beicy is this fixed? Can you update this issue?

@beicy
Copy link
Contributor

beicy commented Aug 10, 2018

@rdzhabarov Yes, this issue can be closed. So for, for convolution, we support non-square padding, non-square kernels and non-square strides.

@nadavrot
Copy link
Contributor

Nice!

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

No branches or pull requests

4 participants