-
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, ONNX] Support gather_nd batch_dims attribute for TF/ONNX #8084
Conversation
2767591
to
e6876dc
Compare
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.
LGTM
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.
LGTM
Our implementation already has an implicit batch dimension. The current implementation is "Given data with shape (X_0, X_1, …, X_{N-1}) and indices with shape (M, Y_0, …, Y_{K-1}), the output will have shape (Y_0, …, Y_{K-1}, X_M, …, X_{N-1}), where M <= N. If M == N, output shape will simply be (Y_0, …, Y_{K-1})." X_M, ..., X_{N-1} is the implicit batch dimension. How does the explicit batch size differ from this. And should we consider unifying the two? |
I don't get what you meant by "implicit batch dimension". X_M, ..., X_{N-1} are axes of the input that are not indexed and thus simply copied. Our current
(There is an open request to add So right now the output is
With
I'm going to update the doc to the following if this makes sense @tkonolige
|
Shouldn't the batch size appear in the output shape? I think it should be |
As the onnx doc says, https://github.com/onnx/onnx/blob/master/docs/Operators.md#GatherND, When I wrote the output shape as |
Ah thats what I was missing. I'd use the expanded definition with X_0... because I think it is clearer for users. |
ffead4b
to
23ac5cf
Compare
Ok since the definition with X_0... only applies when B > 0, I added output shape descriptions for two cases (B > 0 and B == 0). |
thanks @mbrookhart @comaniac @tkonolige |
…che#8084) * Add GatherND batch_dim support * adding tests * test working * improved reference code * refactor ref func * batch dim 2 tests from tf all passed * batch_dim -> batch_dims * add example * minor change * add onnx test * fix onnx version * fix lint * remove move on batch_dims * fix pylint * fix compiler warning * add shape constraint for batch_dim and update doc * make the output shape doc clearer
…che#8084) * Add GatherND batch_dim support * adding tests * test working * improved reference code * refactor ref func * batch dim 2 tests from tf all passed * batch_dim -> batch_dims * add example * minor change * add onnx test * fix onnx version * fix lint * remove move on batch_dims * fix pylint * fix compiler warning * add shape constraint for batch_dim and update doc * make the output shape doc clearer
Similar to #7951 that added
batch_dim
totake
(gather) op, I addedbatch_dims
option togather_nd
that is useful for TF/ONNX.https://www.tensorflow.org/api_docs/python/tf/gather_nd
https://github.com/onnx/onnx/blob/master/docs/Operators.md#GatherND (Opset 12 or higher)
please review @Laurawly @mbrookhart @comaniac @yongwww