-
Notifications
You must be signed in to change notification settings - Fork 3.7k
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
Support GatherND operator in ONNX #2106
Conversation
TF's gather-nd seems to have an extra batch_dims attribute for batched gather-nd … is that required or useful? |
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.
Also could you provide some brief summary about the PR in the PR's description?
Well sure - but the description is same as the title - to support GatherND op in ONNX. Do you mean you want a justification to include this by any chance ? |
Not sure about this - let me take a look. Thanks! |
@gramalingam - I took a look at the |
I personally think it is okay to leave it for future extension if we don't see a use-case for it. |
I think this is a good idea too. We can always version the op later. |
@hariharans29 - Overall, this looks OK. I don't have any specific blocking issues on this. Could you please take a look at the recently approved guideline document for adding new ops and update (if needed) based on that.
|
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. Just one suspicious typo.
onnx/defs/tensor/defs.cc
Outdated
|
||
The output is computed as follows: | ||
|
||
The output tensor can be thought of as filling in the `indices` tensor with the appropriate slices of the input `data`. |
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.
Change to "The output tensor is obtained by mapping each index-tuple in the indices
tensor to the corresponding slice of the input data
"
Let me also add - This operator is the inverse of ScatterND ,since ScatterND has a similar line in it |
By the way: there was a discussion about supporting negative indices in Gather (since pytorch supports that). Do we want to extend this to allow negative indices? |
This is a good question - in this spec version, I explicitly did not allow it and pretty much followed the spec clarification in |
Spoke offline to @spandantiwari and @BowenBao - We will support negative indexing in this op after a consensus on it is reached and we will add negative indexing support in a more streamlined manner across similar ops. For now, it makes sense to stay consistent with |
This change is to support GatherND operator in ONNX.