implement gather #61
implement gather #61
Conversation
src/gather.js
Outdated
| const loc = output.locationFromIndex(i); | ||
| const indicesLoc = loc.slice(axis, axis + indices.rank); | ||
| const selectedInputLoc = loc.slice(0, axis) | ||
| .concat(indices.getValueByLocation(indicesLoc), loc.slice(axis + indices.rank)); |
There was a problem hiding this comment.
Just a FYI, the current implementation in DML will not throw if the values in indices are out of bounds.
The open spec issue for out-of-bounds handling: webmachinelearning/webnn#486
There was a problem hiding this comment.
Thanks for the pointer. webnn-baseline implementation aims at "doing" right thing ((non out-of-bounds access)) and generating the expected test data.
There is a separate task that explicitly checks the out-of-bounds access tracking by issue: webmachinelearning/webnn#244 (comment).
src/gather.js
Outdated
| // if (dimCount <= axis) { | ||
| // continue; | ||
| // } else { | ||
| // here index shoud be set to (rankOutput + dimCount - axis - 1) |
There was a problem hiding this comment.
Please set appropriate indent for this comment.
src/gather.js
Outdated
| const shapeOutput = shapeInput.slice(0, axis).concat(indices.shape, shapeInput.slice(axis + 1)); | ||
| const output = new Tensor(shapeOutput); | ||
|
|
||
| for (let i = 0; i < sizeOfShape(shapeOutput); ++i) { |
There was a problem hiding this comment.
Consider using another variable name? I suppose the 'i' being used in the following comment is different one.
src/gather.js
Outdated
| const loc = output.locationFromIndex(i); | ||
| const indicesLoc = loc.slice(axis, axis + indices.rank); | ||
| const selectedInputLoc = loc.slice(0, axis) | ||
| .concat(indices.getValueByLocation(indicesLoc), loc.slice(axis + indices.rank)); |
There was a problem hiding this comment.
Thanks for the pointer. webnn-baseline implementation aims at "doing" right thing ((non out-of-bounds access)) and generating the expected test data.
There is a separate task that explicitly checks the out-of-bounds access tracking by issue: webmachinelearning/webnn#244 (comment).
| const axisSize = input.shape[axis]; | ||
| for (let i = 0; i < sizeOfShape(indices.shape); ++i) { | ||
| const index = indices.getValueByIndex(i); | ||
| if (!Number.isInteger(index) || index < 0 || index >= axisSize) { |
There was a problem hiding this comment.
The current WebNN DirectML backend allows negative index value. @fdwr
There was a problem hiding this comment.
https://www.w3.org/TR/webnn/#api-mlgraphbuilder-gather
Appears the spec settled on not supporting negative indices, 8. If index is greater than or equal to axisSize, then throw, and that is tracked via webmachinelearning/webnn#484.
I will look through the models I have for any negative indices to see if this will actually matter in practice...
[update] I need some better tools, as this is way too tedious... 🤔⏳
|
@fdwr @huningxin @shiyi9801 I've updated commits to address comments, please take another look, thanks. |
@huningxin @fdwr @wacky6 @shiyi9801 PTAL, thanks