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/TOPI][OP] Add arange op in Relay and TOPI #2621

Merged
merged 10 commits into from
Feb 23, 2019
Merged

Conversation

icemelon
Copy link
Member

@icemelon icemelon commented Feb 18, 2019

Currently I put start, stop, step in Relay attributes since it is required to infer the output shape. Later if Relay supports unknown dimension like Any, we can move them into inputs of arange op instead of attributes.

This PR relies on #2615.

.describe("Start of interval. The interval includes this value.");
TVM_ATTR_FIELD(stop)
.describe("Stop of interval. The interval does not include this value.");
TVM_ATTR_FIELD(start).set_default(make_const(Int(32), 1))
Copy link
Member

Choose a reason for hiding this comment

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

s/start/step

Copy link
Member Author

Choose a reason for hiding this comment

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

Fixed.

relay.arange(5) = [0, 1, 2, 3, 4]
relay.arange(1, 5) = [1, 2, 3, 4]
relay.arange(1, 5, 1.5) = [1, 2.5, 4]
"""
Copy link
Member

Choose a reason for hiding this comment

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

should we consider cases like start > stop and step <= 0, here? I think we probably need to at least warning or raise exceptions for step == 0

Copy link
Member Author

Choose a reason for hiding this comment

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

Added the sanity check in the new commit

CHECK_EQ(types.size(), 1);
const ArangeAttrs* param = attrs.as<ArangeAttrs>();
IndexExpr num_elem = tvm::cast(tvm::Int(32), tvm::ceil(
tvm::cast(tvm::Float(32), param->stop - param->start) / param->step));
Copy link
Member

Choose a reason for hiding this comment

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

CHECK_NE(param->step, 0U) ?

Copy link
Member Author

Choose a reason for hiding this comment

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

step is not necessary to be constant during the compilation time. So probably we should rely on IR to capture this?

Copy link
Member

Choose a reason for hiding this comment

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

I see. Thanks.

std::string name = "tensor",
std::string tag = kInjective) {
Expr num_elem = tvm::cast(tvm::Int(32), tvm::ceil(
tvm::cast(tvm::Float(32), stop - start) / step));
Copy link
Member

Choose a reason for hiding this comment

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

not sure if we need to check if step == 0, probably it is enough if we checked if before

Copy link
Member Author

Choose a reason for hiding this comment

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

divide by 0 should be captured by IR when step is constant.
https://github.com/dmlc/tvm/blob/master/src/lang/ir_operator.cc#L202

@Laurawly
Copy link
Contributor

@zhreshold

@icemelon
Copy link
Member Author

icemelon commented Feb 22, 2019

Hey @yzhliu @zhreshold, could you help review this PR?

@@ -457,6 +457,40 @@ def test_infer_type_prelu():
verify_infer_type_prelu((1, 3, 2, 2), None, 1, (1, 3, 2, 2))
verify_infer_type_prelu((1, 2, 2, 3), None, 3, (1, 2, 2, 3))


def test_arange():
Copy link
Member

Choose a reason for hiding this comment

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

can we also have a relay frontend test from mxnet arange

Copy link
Member Author

Choose a reason for hiding this comment

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

Sure. will add.

Copy link
Member Author

Choose a reason for hiding this comment

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

Added in the new commit.

@@ -249,6 +249,49 @@ def full_like(data, fill_value):
return _make.full_like(data, fill_value)


def arange(stop, start=None, step=1, dtype="float32"):
Copy link
Member

Choose a reason for hiding this comment

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

the doc does not match the arg trick though

Copy link
Member Author

Choose a reason for hiding this comment

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

Made a note in docs in the new commit

IndexExpr num_elem = tvm::cast(tvm::Int(32), tvm::ceil(
tvm::cast(tvm::Float(32), param->stop - param->start) / param->step));
if (const tvm::ir::IntImm* val = num_elem.as<tvm::ir::IntImm>()) {
CHECK_GT(val->value, 0) << "Invalid arange inputs";
Copy link
Member

Choose a reason for hiding this comment

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

suggest to also print related params

Copy link
Member Author

Choose a reason for hiding this comment

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

Fixed in the new commit

Copy link
Member

@zhreshold zhreshold left a comment

Choose a reason for hiding this comment

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

lgtm!

@yzhliu yzhliu merged commit 16b009b into apache:master Feb 23, 2019
@yzhliu
Copy link
Member

yzhliu commented Feb 23, 2019

Thanks all. This is merged.

@icemelon icemelon deleted the arange branch February 24, 2019 02:22
@yzhliu yzhliu mentioned this pull request Mar 2, 2019
28 tasks
wweic pushed a commit to neo-ai/tvm that referenced this pull request Mar 9, 2019
* Add arange op

* Update docs

* Fix bug

* add sanity check in relay and mxnet frontend mapping

* lint

* nits

* pylint

* don't allow empty output from arange

* Remove empty test for arange

* Fix bug and update doc
wweic pushed a commit to neo-ai/tvm that referenced this pull request Mar 12, 2019
* Add arange op

* Update docs

* Fix bug

* add sanity check in relay and mxnet frontend mapping

* lint

* nits

* pylint

* don't allow empty output from arange

* Remove empty test for arange

* Fix bug and update doc
wweic pushed a commit to neo-ai/tvm that referenced this pull request Mar 12, 2019
* Add arange op

* Update docs

* Fix bug

* add sanity check in relay and mxnet frontend mapping

* lint

* nits

* pylint

* don't allow empty output from arange

* Remove empty test for arange

* Fix bug and update doc
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.

6 participants