You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@tvm.apache.org by GitBox <gi...@apache.org> on 2020/11/05 01:36:35 UTC
[GitHub] [incubator-tvm] tkonolige opened a new pull request #6854: [RELAY,TOPI] Add scatter_nd op
tkonolige opened a new pull request #6854:
URL: https://github.com/apache/incubator-tvm/pull/6854
Scatter_nd is the inverse of gather_nd and also happens to be its gradient. The implementation here is not optimized. There are no cpu or gpu specific implementations.
@altanh
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-tvm] altanh commented on pull request #6854: [RELAY,TOPI] Add scatter_nd op
Posted by GitBox <gi...@apache.org>.
altanh commented on pull request #6854:
URL: https://github.com/apache/incubator-tvm/pull/6854#issuecomment-722123984
cc @mbrookhart
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-tvm] tkonolige commented on pull request #6854: [RELAY,TOPI] Add scatter_nd op
Posted by GitBox <gi...@apache.org>.
tkonolige commented on pull request #6854:
URL: https://github.com/apache/incubator-tvm/pull/6854#issuecomment-722540224
@antinucleon @altanh In this implementation, repeated indices are summed instead of having non-deterministic behavior. I put this on the docs for `tvm.topi.scatter`, but I should put it somewhere else too?
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-tvm] tkonolige commented on pull request #6854: [RELAY,TOPI] Add scatter_nd op
Posted by GitBox <gi...@apache.org>.
tkonolige commented on pull request #6854:
URL: https://github.com/apache/incubator-tvm/pull/6854#issuecomment-724260574
@antinucleon Do you want to review this?
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-tvm] altanh commented on a change in pull request #6854: [RELAY,TOPI] Add scatter_nd op
Posted by GitBox <gi...@apache.org>.
altanh commented on a change in pull request #6854:
URL: https://github.com/apache/incubator-tvm/pull/6854#discussion_r517774029
##########
File path: src/relay/analysis/type_solver.cc
##########
@@ -259,10 +259,11 @@ class TypeSolver::Unifier : public TypeFunctor<Type(const Type&, const Type&)> {
if (mismatches.size() != 0) {
auto err = Diagnostic::Error(this->span);
- err << "in particular ";
+ err << "The Relay type checker is unable to show the following types match.\n";
Review comment:
The original error formatting may have been intentional to show some kind of "traceback" style reporting, but I may be wrong. cc @jroesch who has a better idea
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-tvm] tkonolige commented on a change in pull request #6854: [RELAY,TOPI] Add scatter_nd op
Posted by GitBox <gi...@apache.org>.
tkonolige commented on a change in pull request #6854:
URL: https://github.com/apache/incubator-tvm/pull/6854#discussion_r518246012
##########
File path: python/tvm/relay/op/_tensor_grad.py
##########
@@ -803,3 +805,15 @@ def arange_grad(orig, grad):
grad_step = cast_like(_sum(grad_step), step)
return [grad_start, grad_stop, grad_step]
+
+
+@register_gradient("gather_nd")
+def gather_nd_grad(orig, grad):
+ data, indices = orig.args
+ return [scatter_nd(grad, indices, data.checked_type.concrete_shape), zeros_like(indices)]
+
+
+# @register_gradient("scatter_nd")
Review comment:
I'm getting some really weird results when using gather_nd as the gradient. Values are like 1e303.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-tvm] altanh commented on a change in pull request #6854: [RELAY,TOPI] Add scatter_nd op
Posted by GitBox <gi...@apache.org>.
altanh commented on a change in pull request #6854:
URL: https://github.com/apache/incubator-tvm/pull/6854#discussion_r517774800
##########
File path: python/tvm/relay/op/transform.py
##########
@@ -308,6 +308,30 @@ def scatter_add(data, indices, updates, axis):
return _make.scatter_add(data, indices, updates, axis)
+def scatter_nd(data, indices, out_shape):
+ """Scatter values from an array.
+
+ See :py:func:`tvm.topi.scatter` for how data is scattered.
Review comment:
is this doc ref correct?
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-tvm] altanh commented on pull request #6854: [RELAY,TOPI] Add scatter_nd op
Posted by GitBox <gi...@apache.org>.
altanh commented on pull request #6854:
URL: https://github.com/apache/incubator-tvm/pull/6854#issuecomment-725614013
We found some bugs in the shape relation and compute, @tkonolige will push a fix and test cases shortly
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-tvm] altanh commented on pull request #6854: [RELAY,TOPI] Add scatter_nd op
Posted by GitBox <gi...@apache.org>.
altanh commented on pull request #6854:
URL: https://github.com/apache/incubator-tvm/pull/6854#issuecomment-722123621
Can you try to fix the CI errors? Left some comments but overall looks really good to me, +1 on the thorough documentation!
For reference, this is based off of MXNet's `scatter_nd` (https://mxnet.apache.org/versions/1.6/api/r/docs/api/mx.symbol.scatter_nd.html).
Given the warning about non-determinism there we may want to make a comment here as well.
Do the semantics of MXNet's `scatter_nd` differ from Tensorflow (https://www.tensorflow.org/api_docs/python/tf/scatter_nd) at all?
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-tvm] tkonolige commented on pull request #6854: [RELAY,TOPI] Add scatter_nd op
Posted by GitBox <gi...@apache.org>.
tkonolige commented on pull request #6854:
URL: https://github.com/apache/incubator-tvm/pull/6854#issuecomment-722665085
@mbrookhart I'd like to work on the cuda implementation separately. I think there is a bit of work to do there.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-tvm] tkonolige commented on pull request #6854: [RELAY,TOPI] Add scatter_nd op
Posted by GitBox <gi...@apache.org>.
tkonolige commented on pull request #6854:
URL: https://github.com/apache/incubator-tvm/pull/6854#issuecomment-728343498
@tqchen This PR is all ready to go. Is there a reviewer you could suggest?
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-tvm] antinucleon commented on pull request #6854: [RELAY,TOPI] Add scatter_nd op
Posted by GitBox <gi...@apache.org>.
antinucleon commented on pull request #6854:
URL: https://github.com/apache/incubator-tvm/pull/6854#issuecomment-722140534
> Can you try to fix the CI errors? Left some comments but overall looks really good to me, +1 on the thorough documentation!
>
> For reference, this is based off of MXNet's `scatter_nd` (https://mxnet.apache.org/versions/1.6/api/r/docs/api/mx.symbol.scatter_nd.html).
> Given the warning about non-determinism there we may want to make a comment here as well.
>
> Do the semantics of MXNet's `scatter_nd` differ from Tensorflow (https://www.tensorflow.org/api_docs/python/tf/scatter_nd) at all?
MXNet `scatter_nd` semantic is a little bit different to TF's semantic. Similarly, @tkonolige points out this thread: https://discuss.tvm.apache.org/t/gather-nd-semantics/6243/6 As long as scatter is not a numpy op, if it is only a transpose difference I think it is probably fine.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-tvm] altanh commented on a change in pull request #6854: [RELAY,TOPI] Add scatter_nd op
Posted by GitBox <gi...@apache.org>.
altanh commented on a change in pull request #6854:
URL: https://github.com/apache/incubator-tvm/pull/6854#discussion_r517774679
##########
File path: python/tvm/relay/op/_tensor_grad.py
##########
@@ -803,3 +805,15 @@ def arange_grad(orig, grad):
grad_step = cast_like(_sum(grad_step), step)
return [grad_start, grad_stop, grad_step]
+
+
+@register_gradient("gather_nd")
+def gather_nd_grad(orig, grad):
+ data, indices = orig.args
+ return [scatter_nd(grad, indices, data.checked_type.concrete_shape), zeros_like(indices)]
+
+
+# @register_gradient("scatter_nd")
Review comment:
uncomment or remove, is something wrong with it?
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-tvm] tkonolige commented on a change in pull request #6854: [RELAY,TOPI] Add scatter_nd op
Posted by GitBox <gi...@apache.org>.
tkonolige commented on a change in pull request #6854:
URL: https://github.com/apache/incubator-tvm/pull/6854#discussion_r518246356
##########
File path: python/tvm/relay/op/transform.py
##########
@@ -308,6 +308,30 @@ def scatter_add(data, indices, updates, axis):
return _make.scatter_add(data, indices, updates, axis)
+def scatter_nd(data, indices, out_shape):
+ """Scatter values from an array.
+
+ See :py:func:`tvm.topi.scatter` for how data is scattered.
Review comment:
I believe so.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-tvm] altanh commented on a change in pull request #6854: [RELAY,TOPI] Add scatter_nd op
Posted by GitBox <gi...@apache.org>.
altanh commented on a change in pull request #6854:
URL: https://github.com/apache/incubator-tvm/pull/6854#discussion_r517774557
##########
File path: tests/python/relay/test_op_grad_level3.py
##########
@@ -117,5 +117,23 @@ def test_arange_grad():
check_grad(fwd_func, inputs=values)
+def test_gather_nd_grad():
+ data = relay.var("data", relay.TensorType((2, 3), "float64"))
+ indices = relay.var("indices", relay.TensorType((2, 4), "int64"))
+ fwd = relay.Function([data, indices], relay.gather_nd(data, indices))
+ data_np = np.random.rand(2, 3)
+ indices_np = np.array([[0, 2, 1, 0], [0, 1, 2, 1]])
+ check_grad(fwd, inputs=[data_np, indices_np], test_inputs=[indices_np])
+
+
+# def test_scatter_nd_grad():
Review comment:
uncomment or remove
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-tvm] altanh commented on a change in pull request #6854: [RELAY,TOPI] Add scatter_nd op
Posted by GitBox <gi...@apache.org>.
altanh commented on a change in pull request #6854:
URL: https://github.com/apache/incubator-tvm/pull/6854#discussion_r517773679
##########
File path: python/tvm/testing.py
##########
@@ -714,4 +714,33 @@ def func(f):
return wrap(args)
+def compare_numpy_tvm(inputs, output, target, ctx, compute, schedule):
Review comment:
I wonder if this would better live in `topi.testing` since it depends on TOPI compute and schedule.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-tvm] altanh commented on pull request #6854: [RELAY,TOPI] Add scatter_nd op
Posted by GitBox <gi...@apache.org>.
altanh commented on pull request #6854:
URL: https://github.com/apache/incubator-tvm/pull/6854#issuecomment-722618737
Hmm yeah seems like the gradient unit test is failing, can you investigate? Also I recommend reverting the diagnostics error message change, looks like there is a unit test that depends on the specific error message.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-tvm] tkonolige commented on pull request #6854: [RELAY,TOPI] Add scatter_nd op
Posted by GitBox <gi...@apache.org>.
tkonolige commented on pull request #6854:
URL: https://github.com/apache/incubator-tvm/pull/6854#issuecomment-723335052
@mbrookhart Actually, you've got your wish. I've added a simple x86 and cuda implementation.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org