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

[Relay][Frontend] Add reverse op to relay #2800

Merged
merged 6 commits into from
Mar 13, 2019
Merged

Conversation

Laurawly
Copy link
Contributor

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.

@Laurawly Laurawly requested a review from icemelon March 12, 2019 22:23
TVM_ATTR_FIELD(axis).set_default(NullValue<Integer>())
.describe(" The axis along which to reverse elements.");
}
}; // struct RepeatAttrs
Copy link
Member

Choose a reason for hiding this comment

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

Typo. Change to ReverseAttrs.

intrp = relay.create_executor(kind, ctx=ctx, target=target)
op_res = intrp.evaluate(func)(x_data)
tvm.testing.assert_allclose(op_res.asnumpy(), ref_res, rtol=1e-5)
verify_reverse((2, 3, 4), 1 )
Copy link
Member

Choose a reason for hiding this comment

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

Remove the extra space

const auto* param = attrs.as<ReverseAttrs>();
const int ndim = static_cast<int>(data->shape.size());
const int axis = param->axis;
CHECK(-ndim - 1 <= axis && axis <= ndim)
Copy link
Member

Choose a reason for hiding this comment

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

Should it be axis < ndim?

}

Expr MakeReverse(Expr data,
int axis) {
Copy link
Member

Choose a reason for hiding this comment

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

Fix indent

Integer axis;
TVM_DECLARE_ATTRS(ReverseAttrs, "relay.attrs.ReverseAttrs") {
TVM_ATTR_FIELD(axis).set_default(NullValue<Integer>())
.describe(" The axis along which to reverse elements.");
Copy link
Member

Choose a reason for hiding this comment

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

Remove the extra space

The axis along which to reverse elements.

.. note::
Reverse and flip are equivalent.
Copy link
Member

Choose a reason for hiding this comment

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

I don't think we need this note since we don't have flip op in Relay.

--------
.. code-block:: python

x = [[1, 2], [3, 4]]
Copy link
Member

Choose a reason for hiding this comment

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

Use float value to be consistent

for (int i = 0; i < ndim; ++i) {
oshape.emplace_back(data->shape[i]);
}
reporter->Assign(types[1], TensorTypeNode::make(oshape, data->dtype));
Copy link
Member

Choose a reason for hiding this comment

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

You can directly use reporter->Assign(types[1], types[0]);. No need to create oshape.

const auto* param = attrs.as<ReverseAttrs>();
const int ndim = static_cast<int>(data->shape.size());
const int axis = param->axis;
CHECK(-ndim - 1 < axis && axis < ndim)
Copy link
Member

Choose a reason for hiding this comment

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

We can just use -ndim <= axis to simplify the expr.

@icemelon icemelon merged commit 4d09fc4 into apache:master Mar 13, 2019
@icemelon
Copy link
Member

Thanks @Laurawly. This is now merged.

wweic pushed a commit to wweic/tvm that referenced this pull request Mar 20, 2019
* start adding reverse

* reverse updated

* reverse uses topi::flip

* typo fixed

* comment addressed

* exp simplified
wweic pushed a commit to neo-ai/tvm that referenced this pull request Mar 20, 2019
* start adding reverse

* reverse updated

* reverse uses topi::flip

* typo fixed

* comment addressed

* exp simplified
@Laurawly Laurawly deleted the reverse branch March 22, 2019 20:35
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.

2 participants