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][Op] concatenate, reshape, transpose, copy #1847

Merged
merged 15 commits into from
Oct 7, 2018
Merged

[Relay][Op] concatenate, reshape, transpose, copy #1847

merged 15 commits into from
Oct 7, 2018

Conversation

junrushao
Copy link
Member

@junrushao junrushao commented Oct 6, 2018

OP TODO

  • [level 1] relay.concatenate
  • [level 3] relay.copy
  • [level 3] relay.transpose
  • [level 3] relay.reshape

Misc TODO

  • Rebase
  • Check sanity check coverage
  • Find right file to put these operators
  • Re-check docs

Issues & discussion

  • Numpy consistency: numpy.concatenate accepts axis=None, in which case numpy flattens all input arrays before doing concat, while we do not support axis=None.
  • Numpy consistency: numpy.reshape is a subset of our reshape. I think it is fine (or even better).
  • Flexibility: is it okay that transpose directly convert Array<IndexExpr> axes to vector<int>?

Related issue: #1799

} else {
std::vector<int> axis_used(ndim, 0);
for (const IndexExpr& e: axes) {
int axis = _helper::ToInt(e);
Copy link
Member

Choose a reason for hiding this comment

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

there is a function as_const_int, we can also populate common helper functions in op_utils.h

@tqchen
Copy link
Member

tqchen commented Oct 6, 2018

It is OK(and encouraged when we intended to do so) to directly require IndexExpr to be int when they are in array, we can do

#include <tvm/ir_operator.h>

for (size_t i = 0; i < axes.size(); ++i) {
   const int64_t* paxis = as_const_int(axes[i]);
   CHECK(paxis != nullptr);
    ...
}

@tqchen
Copy link
Member

tqchen commented Oct 6, 2018

  • concatenate: I think it is fine for now to not support axis=None and make clear error to the user
  • reshape: as long as the numpy part is consistent, it is fine

@junrushao junrushao changed the title [Relay][Op][WIP] concatenate, reshape, transpose, copy [Relay][Op] concatenate, reshape, transpose, copy Oct 6, 2018
@junrushao
Copy link
Member Author

@tqchen @srkreddy1238 Hey folks could you help review this PR?

<< ", and ndim = " << ndim;
axis = axis < 0 ? ndim + axis : axis;
// Calculate shape
std::vector<IndexExpr> oshape;
Copy link
Member

@tqchen tqchen Oct 6, 2018

Choose a reason for hiding this comment

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

Let us make it as a helper function AsVector<T>(const Array<T>&) and put it under op_common.h in src/relay/op

# let's mimic a batch of sequences
x = ib.param("x", relay.ty.TensorType((n, t, d), "float32"))
y = ib.param("y", relay.ty.TensorType((n, t, d), "float32"))
with ib.function(x, y) as func:
Copy link
Member

Choose a reason for hiding this comment

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

please test several case of axis

@@ -345,3 +330,41 @@ def ones_like(data):
The computed result.
"""
return _make.ones_like(data)

def concatenate(data, axis=0):
Copy link
Member

Choose a reason for hiding this comment

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

Let us consider remove default axis in concatenate for now, The main reason is that historically concat usually defaults to axis=1, this new default will likely cause some confusion.

@junrushao
Copy link
Member Author

@tqchen hey could you help review this again at your convenience? Rebasing many times seems painful to me.

@tqchen
Copy link
Member

tqchen commented Oct 7, 2018

The changes LGTM, let us wait the CI and then merge it in

@tqchen tqchen merged commit 710af08 into apache:master Oct 7, 2018
FrozenGene pushed a commit to FrozenGene/tvm that referenced this pull request Dec 27, 2018
@junrushao junrushao deleted the relay-op-3 branch January 6, 2022 02:55
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