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] GetItem #1861

Merged
merged 9 commits into from
Oct 10, 2018
Merged

[Relay] GetItem #1861

merged 9 commits into from
Oct 10, 2018

Conversation

MarisaKirisame
Copy link
Contributor

GetItem is a relay construct for getting field out of a tuple

@MarisaKirisame
Copy link
Contributor Author

@junrushao1994 @srkreddy1238 @masahi please review.

/*! \brief The tuple */
Expr tuple;
/*! \brief which value to get */
size_t field;
Copy link
Member

Choose a reason for hiding this comment

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

change to int index;

Copy link
Member

Choose a reason for hiding this comment

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

the main reason is that usually if we want to serialize the model, the type should have fixed size across platform(int32, or int64). index is a more commonly used term

@@ -378,6 +376,23 @@ class IfNode : public ExprNode {

RELAY_DEFINE_NODE_REF(If, IfNode, Expr);

/*! \brief Get a field out of a tuple. */
class GetItem;
Copy link
Member

Choose a reason for hiding this comment

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

GetItem->TupleGetItem

Copy link
Member

@junrushao junrushao left a comment

Choose a reason for hiding this comment

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

Looks good to me

@tqchen tqchen merged commit 6616355 into apache:master Oct 10, 2018
@MarisaKirisame MarisaKirisame deleted the getitem branch October 10, 2018 21:26
@MarisaKirisame
Copy link
Contributor Author

@junrushao1994 thx for reviewing!
@jroesch the relay doc/evaluator should be updated with tuplegetitem
@joshpoll the relay prettyprinter/parser/text format should be updated with tuplegetitem
I can take the responsibility of adding tuplegetitem if you two are busy - in that case I can make pr to you two's branch.

FrozenGene pushed a commit to FrozenGene/tvm that referenced this pull request Dec 27, 2018
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.

4 participants