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.

Sign up for GitHub

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

Jump to bottom

[RELAY,TOPI] Add scatter_nd op #6854

Merged
merged 13 commits into from
Dec 1, 2020
Merged

[RELAY,TOPI] Add scatter_nd op #6854

merged 13 commits into from
Dec 1, 2020

Conversation

tkonolige
Copy link
Contributor

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

altanh
altanh reviewed Nov 5, 2020
View reviewed changes
python/tvm/testing.py Outdated Show resolved Hide resolved
altanh
altanh reviewed Nov 5, 2020
View reviewed changes
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";
Copy link
Contributor

Choose a reason for hiding this comment

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

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

@altanh
Copy link
Contributor

altanh commented Nov 5, 2020

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?

@altanh
Copy link
Contributor

altanh commented Nov 5, 2020

cc @mbrookhart

@antinucleon
Copy link
Contributor

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.

@tkonolige
Copy link
Contributor Author

@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?

mbrookhart
mbrookhart reviewed Nov 5, 2020
View reviewed changes
Copy link
Contributor

@mbrookhart mbrookhart left a comment

Choose a reason for hiding this comment

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

It looks like the gather_nd_grad unit test has a type error?

How hard would it be to extend this to cuda as part of this PR? Since you're already using IR builder, you might be able to join them pretty easiliy.

@altanh
Copy link
Contributor

altanh commented Nov 5, 2020

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.

@tkonolige
Copy link
Contributor Author

@mbrookhart I'd like to work on the cuda implementation separately. I think there is a bit of work to do there.

@tkonolige
Copy link
Contributor Author

@mbrookhart Actually, you've got your wish. I've added a simple x86 and cuda implementation.

@tkonolige
Copy link
Contributor Author

@antinucleon Do you want to review this?

tkonolige and others added 11 commits November 11, 2020 10:30
@tkonolige
[RELAY,TOPI] Add scatter_nd op
e5f616b 
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.
@tkonolige
formatting
3aa5bde
@tkonolige
Fix tests
dff3644
@tkonolige
formatting
4373eba
@tkonolige
specify types on test
1d9d1f9
@tkonolige
Fix grad test
288873f
@tkonolige
scatter_nd cuda impl
60d94d7
@tkonolige
cuda impl
557b915
@tkonolige
x86 impl
2431807
@tkonolige
formatting
0593227
@tkonolige
fix shape rel
8cf300f
@altanh
Copy link
Contributor

altanh commented Nov 11, 2020

We found some bugs in the shape relation and compute, @tkonolige will push a fix and test cases shortly

@tkonolige
Copy link
Contributor Author

@tqchen This PR is all ready to go. Is there a reviewer you could suggest?

@jroesch jroesch merged commit 0421efb into apache:main Dec 1, 2020
trevor-m pushed a commit to trevor-m/tvm that referenced this pull request Dec 2, 2020
@tkonolige
[RELAY,TOPI] Add scatter_nd op (apache#6854)
4c4de30 
* [RELAY,TOPI] Add scatter_nd op

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.

* formatting

* Fix tests

* formatting

* specify types on test

* Fix grad test

* scatter_nd cuda impl

* cuda impl

* x86 impl

* formatting

* fix shape rel

* fix tests

* formatting
trevor-m pushed a commit to trevor-m/tvm that referenced this pull request Dec 4, 2020
@tkonolige
[RELAY,TOPI] Add scatter_nd op (apache#6854)
4ef456c 
* [RELAY,TOPI] Add scatter_nd op

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.

* formatting

* Fix tests

* formatting

* specify types on test

* Fix grad test

* scatter_nd cuda impl

* cuda impl

* x86 impl

* formatting

* fix shape rel

* fix tests

* formatting
trevor-m pushed a commit to neo-ai/tvm that referenced this pull request Dec 4, 2020
@tkonolige @trevor-m
[RELAY,TOPI] Add scatter_nd op (apache#6854)
831fdfb 
* [RELAY,TOPI] Add scatter_nd op

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.

* formatting

* Fix tests

* formatting

* specify types on test

* Fix grad test

* scatter_nd cuda impl

* cuda impl

* x86 impl

* formatting

* fix shape rel

* fix tests

* formatting
@masahi masahi mentioned this pull request Jan 8, 2021
Merged
@junrushao junrushao mentioned this pull request Nov 1, 2021
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@altanh altanh altanh left review comments

@mbrookhart mbrookhart mbrookhart approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@tkonolige @altanh @antinucleon @mbrookhart @jroesch