[RELAY]Slice_like support #2014
Conversation
|
@srkreddy1238 @nishi-t @slyubomirsky @yuruofeifei @MarisaKirisame Could you please help in reviewing this PR. TIA |
python/tvm/relay/op/transform.py
Outdated
| shape_like : tuple of int | ||
| The new shape. | ||
|
|
||
| axes : tuple of int, optional |
There was a problem hiding this comment.
Optional[Tuple[int]]
python/tvm/relay/op/transform.py
Outdated
| data : relay.Expr | ||
| The source array. | ||
|
|
||
| shape_like : tuple of int |
python/tvm/relay/op/transform.py
Outdated
|
|
||
| Parameters | ||
| ---------- | ||
| data : relay.Expr |
include/tvm/relay/attrs/transform.h
Outdated
| TVM_ATTR_FIELD(axes).set_default(Array<Integer>()) | ||
| .describe("List of axes on which input data will be sliced according to the " | ||
| "corresponding size of the second input. By default will slice " | ||
| "on all axes. Negative axes are supported."); |
There was a problem hiding this comment.
Negative axes are supported -> Negative axes mean counting in reverse.
Only slightly longer, and convey the exact meaning
src/relay/op/tensor/transform.cc
Outdated
| CHECK(data != nullptr); | ||
|
|
||
| const auto* target = types[1].as<TensorTypeNode>(); | ||
| CHECK(target != nullptr); |
There was a problem hiding this comment.
return false on null, as it might be resolved later.
src/relay/op/tensor/transform.cc
Outdated
| const TypeReporter& reporter) { | ||
| CHECK_EQ(types.size(), 3); | ||
| const auto* data = types[0].as<TensorTypeNode>(); | ||
| CHECK(data != nullptr); |
There was a problem hiding this comment.
return false on null, as it might be resolved later.
src/relay/op/tensor/transform.cc
Outdated
| } | ||
| CHECK(reporter->Assert(i < make_const(Int(64), target_shape.size()))) | ||
| << "Axis " << i << " exceeds dimension " | ||
| << target_shape.size()<< " of target_shape."; |
siju-samuel
force-pushed
the
relay_slice_like
branch
2 times, most recently
from
43de247
to
c0a8739
Compare
|
Since slice_like is not a conventional numpy op, let us put its support level as 10 for now, to mark it as experimental |
siju-samuel
force-pushed
the
relay_slice_like
branch
from
b14bb45
to
e71b52e
Compare
src/relay/op/tensor/transform.cc
Outdated
| const Array<IndexExpr> target_shape = target->shape; | ||
| std::vector<IndexExpr>&& oshape = AsVector(dshape); | ||
|
|
||
| if (!param->axes.defined() || param->axes.size() == 0) { |
There was a problem hiding this comment.
do not support cases of axes.size() == 0, just us None to indicate everything
There was a problem hiding this comment.
@tqchen if we dont support 'axes.size() == 0,' the behaviour will be different for axes=None and axes=[]. I think both should behave the same way. What is your opinion?
There was a problem hiding this comment.
In the numpy semantics, axes=None have special meaning, while axes=() really means the axes set is empty, and user might need an error message when this occurs
siju-samuel
force-pushed
the
relay_slice_like
branch
from
ea35173
to
9a1a656
Compare
|
@tqchen review comments are resolved. |
|
@MarisaKirisame @vinx13 Please review this PR. |
siju-samuel
force-pushed
the
relay_slice_like
branch
2 times, most recently
from
4fcef04
to
b1c690f
Compare
src/relay/op/tensor/transform.cc
Outdated
| } | ||
|
|
||
| const auto param = attrs.as<SliceLikeAttrs>(); | ||
| if (param == nullptr) { |
There was a problem hiding this comment.
CHECK(param != nullptr);
as we always expect param to exist
src/relay/op/tensor/transform.cc
Outdated
| } else { | ||
| CHECK(param->axes.size() != 0) << "Axes cannot be empty."; | ||
| for (Integer i : param->axes) { | ||
| if (reporter->Assert(i < make_const(Int(64), 0))) { |
There was a problem hiding this comment.
We cannot use Assert for checking symbolic conditions. since it is already Integer, use instead
for (Integer val : param->axes) {
int axis = val->value;
if (axis < 0) {
....
}
}
src/relay/op/tensor/transform.cc
Outdated
| if (reporter->Assert(i < make_const(Int(64), 0))) { | ||
| i += make_const(Int(64), dshape.size()); | ||
| } | ||
| CHECK(reporter->Assert(i < make_const(Int(64), target_shape.size()))) |
|
Some followup comment, I have noticed some common problems in the code: Assert get misused quite often to get facts out of symbolic relations, we should not do that. Remember that Assert can optionally trigger an error if the problem occurs, and should not be sued to elicit fact from the solver. If we expect something is concrete integer, simply get things out in comparison |
siju-samuel
force-pushed
the
relay_slice_like
branch
from
b1c690f
to
6b3ed22
Compare
|
Thanks, @siju-samuel @MarisaKirisame this is merged |
#1799
Thanks for contributing to TVM! Please refer to guideline https://docs.tvm.ai/contribute/ for useful information and tips. After the pull request is submitted, please request code reviews from Reviewers.