Strided_slice added in NNVM #1318
Conversation
| } | ||
|
|
||
| return Array<Tensor>{ topi::strided_slice(inputs[0], begin, end, stride) }; | ||
| }) |
There was a problem hiding this comment.
add set_attr("FCorrectLayout", ElemwiseArbitraryLayout<1, 1>)
nnvm/include/nnvm/top/tensor.h
Outdated
| .describe("Indices for begin of slice"); | ||
| DMLC_DECLARE_FIELD(end) | ||
| .describe("Indices for end of the slice"); | ||
| DMLC_DECLARE_FIELD(stride) |
There was a problem hiding this comment.
For the stride, should add set_default like this:
.set_default(Tuple<int64_t>()) like MXNet did. Because stride is an optional value. Otherwise, users must provide this value for stride in the frontend (not the same as your unit testing, just _sym.strided_slice(insym, begin=begin, end=end) ), which is not the same meaning we want to do.
PariksheetPinjari909
force-pushed
the
stridedslice_nnvm
branch
from
fff4bb7
to
ab68803
Compare
|
@FrozenGene , @kevinthesun : your comments are handled please check, thanks! |
|
@PariksheetPinjari909 I have no other comments now. I have used this PR into my real model, which is currently no problem. Thanks. |
|
@FrozenGene @kevinthesun @tqchen @PariksheetPinjari909 Ref. https://github.com/onnx/onnx/blob/master/docs/Operators.md#Slice Current implementation fills the begin, end, strides only at the end. But doesn't support choosing arbitrary axis. Please correct me if I am wrong. |
|
@srkreddy1238 I haven't thought of one situation onnx's axes can do but our slice can not do. If you want to choose one specify axis, you could use |
|
Our implementation is ref from MXNet and tensorflow is a subset of it. ONNX differs slightly here. How do we specify begin, end only for axis 1 here for a 4D tensor (axis 0, 2, 3 should take full range)? @tqchen any advice ? |
|
One thing we need to support is the case when some of the end is None(which means inclusive, for cases like X[0:]), as long as that is supported, it can support any axis. |
|
How do we pass None here ? onnx use very high value (like INT_MAX) to indicate end. |
|
|
|
explicit checking for int_max and not applying slice makes sense |
|
begin(1, indicates begin offset for axis 0. |
| begin_vec.push_back(0); | ||
| } | ||
|
|
||
| std::vector<int64_t> end_vec; |
There was a problem hiding this comment.
Handle INT_MAX for end
There was a problem hiding this comment.
I had considered this scenario while implementation, and concluded that the functionality can be achieved using the current implementation, which i did in case of Tensorflow case(had many other parameters like ellipsis_mask, new_axis_mask, shrink_axis_mask)(PR coming soon), and it works fine, similarly can be done for ONNX also.
NOTE: in Tensorflow ellipsis_mask is nothing but similar to axis input case in ONNX.
I have gone through onnx slice operator, my understanding is INT_MAX they will use when the size in the particular dimension is unknown, which is not the case in NNVM.
Apart from that our current implementation can handle INT_MAX input perfectly, it will strip down end value to the max size of the dimension. In my opinion, we should not modify the base topi operators based on each frontends, provided the functionality can be achieved by modifying the inputs to the operator.
Please let me know @tqchen , @srkreddy1238 , @FrozenGene , your opinions. Will wait for your feedback.
There was a problem hiding this comment.
I don't think apply INT_MAX for slice op making sense. INT_MAX is for unknown dimension, which is not the things NNVM should consider. I translated many models with unknown dimension (such as ssd-mobilenet / Deeplab V3 tensorflow model's preprocess input), for this situation, the users should provide the concrete shape, not leaving this to NNVM.
There was a problem hiding this comment.
It's not the dimension which is unknown here. It's the range inside the dimension.
INT_MAX here indicates to use max available range from input.
nnvm/src/top/tensor/transform.cc
Outdated
| } | ||
|
|
||
| std::vector<int64_t> stride_vec; | ||
| if (param.stride.ndim() != 0) { |
There was a problem hiding this comment.
stride has a default initialization. This check may not be needed.
| } | ||
|
|
||
| NNVM_REGISTER_OP(strided_slice) | ||
| .describe(R"code(Strided slice of an array. |
There was a problem hiding this comment.
Please elaborate the description with examples.
| verify_strided_slice((3, 4, 3), [1, -1, 0], [4, -5, 3], [2, -1, 1]) | ||
| verify_strided_slice((3, 4, 3), [1, 0, 0], [2, 2, 3], [1, 1, 2]) | ||
| verify_strided_slice((3, 4, 3), [1, -1, 0], [2, -3, 3], [1, -1, 1]) | ||
| verify_strided_slice((3, 4, 3), [1, 1, 0], [4, 4, 3]) |
There was a problem hiding this comment.
Could add test cases for INT_MAX and another where len(begin) != len(end).
|
@srkreddy1238 , all comments are handled now. |
Strided_slice added in NNVM