-
Notifications
You must be signed in to change notification settings - Fork 3.5k
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][OP] Support MXNet-style attributes for reshape_like #6851
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good overall,, only minor changes
src/relay/op/make_op.h
Outdated
@@ -62,6 +62,9 @@ Expr MakeRepeat(Expr data, int repeats, int axis); | |||
|
|||
Expr MakeReshape(Expr data, Array<Integer> newshape); | |||
|
|||
Expr MakeReshapeLike(Expr lhs, Expr rhs, int64_t lhs_begin, Integer lhs_end, int64_t rhs_begin, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why {lhs,rhs}_begin is int64_t and {lhs,rhs}_end is Integer?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I did this because the beginning index (in both cases) must always be an integer, but the end index can be None
which means I must use a nullable Integer
wrapper. I could make everything Integer
and check that beginning is always defined. I did feel a bit weird using int64_t
directly since everything else seem to use int
but the value wrapped by Integer
is int64_t
so that's why I chose it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I noticed that the Attrs I defined uses int
and not int64_t
so I'll probably just use int
since other code mostly uses it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Overall looks good, just one style comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for addressing the comments! LGTM
Thanks @altanh @tkonolige @electriclilies @jroesch @giuseros! It is now merged :-) |
This PR adds MXNet-style shape slicing attributes for
reshape_like
, to enable reshaping part of a shape (using part of another shape). In particular, this addslhs_begin, lhs_end, rhs_begin, rhs_end
.cc @kevinthesun @icemelon9 @junrushao1994