[Relay][TOPI] operator All #3124
Conversation
|
LGTM just waiting on someone with more area expertise. |
|
|
||
| Example:: | ||
|
|
||
| data = [[[ True, True, True], |
There was a problem hiding this comment.
Does this operator allow numerical type?We'd better update the doc to reflect this.
There was a problem hiding this comment.
just allow boolean tensor, have updated the doc
|
Overall lgtm. A minor comment about doc. |
python/tvm/relay/op/reduce.py
Outdated
|
|
||
| exclude : bool | ||
| If `exclude` is true, reduction will be performed on the axes that are | ||
| NOT in axis instead. |
| exclude : bool | ||
| If `exclude` is true, reduction will be performed on the axes that are | ||
| NOT in axis instead. | ||
|
|
There was a problem hiding this comment.
Could you also add the example in cpp to python as well?
docs/langref/relay_op.rst
Outdated
| @@ -309,6 +310,7 @@ Level 6 Definitions | |||
|
|
|||
| Level 10 Definitions | |||
| -------------------- | |||
| .. autofunction:: tvm.relay.all | |||
There was a problem hiding this comment.
Let's move all to level 4 op since it's a standard numpy op.
| op_res = intrp.evaluate(func)(x_data) | ||
| tvm.testing.assert_allclose(op_res.asnumpy(), ref_res, rtol=1e-5) | ||
| verify_contrib_all((2, 3, 4), axis=(0,)) | ||
| verify_contrib_all((2, 3, 4, 5, 6), axis=(2, 3), keepdims=True) |
There was a problem hiding this comment.
Add axis=None and exclude test case.
There was a problem hiding this comment.
Also axis is negative test cases
topi/include/topi/reduction.h
Outdated
| * over a given axis | ||
| * | ||
| * \param data The input tensor | ||
| * \param axis The axis to perform logical AND over. If axis is empty, the |
| op_res = intrp.evaluate(func)(x_data) | ||
| tvm.testing.assert_allclose(op_res.asnumpy(), ref_res, rtol=1e-5) | ||
| verify_contrib_all((2, 3, 4), axis=(0,)) | ||
| verify_contrib_all((2, 3, 4, 5, 6), axis=(2, 3), keepdims=True) |
There was a problem hiding this comment.
Also axis is negative test cases
|
@yongwww what's the status on this? |
|
@jroesch I'm on vacation this week, will update the pr to incorporate comments by this weekend |
Co-Authored-By: yongwww <[email protected]>
|
@icemelon9 @jroesch please take another look and https://docs.tvm.ai/contribute/code_review.html#approve-and-request-changes-explicitly |
icemelon
added
status: accepted
and removed
status: need review
status: need update
|
Thanks everyone. This is now merged. |
* [Relay][TOPI] operator All * Update tests/python/frontend/tensorflow/test_forward.py Co-Authored-By: yongwww <[email protected]> * fix comments * change to level 4
* [Relay][TOPI] operator All * Update tests/python/frontend/tensorflow/test_forward.py Co-Authored-By: yongwww <[email protected]> * fix comments * change to level 4
Add op
Allin TOPI, RELAY and TF frontend, all is used in numpy and tensorflow, need it to support tf model ssd/fast-rcnn/mask-rcnn. I will change it to relay.contrib.all, please suggest if you guys have any thought.@yzhliu @icemelon9 @Laurawly @zhiics @jroesch @srkreddy1238 @tqchen