Implement pad op #47
Implement pad op #47
Conversation
fdwr
left a comment
fdwr
left a comment
There was a problem hiding this comment.
I need to stare at getMappedLocation some more tomorrow, but some partial feedback for now.
src/pad.js
Outdated
| const outputShape = input.shape.map((v, i) => v + beginningPadding[i] + endingPadding[i]); | ||
| let output = new Tensor(outputShape); | ||
| for (let i = 0; i < output.size; ++i) { | ||
| output = updateOutput(i, input, output, beginningPadding, mode, value); |
There was a problem hiding this comment.
Is this correct to overwrite output? Doesn't updateOutput already call setValueByIndex? Otherwise line 108 will only return the last loop value.
There was a problem hiding this comment.
Yes, this for loop is to set each element of output tensor by index.
Maybe the function name updateOutput confuse you, how about renaming it as updateElement or something, any suggestions? Thanks.
There was a problem hiding this comment.
(I missed this notification, and the CR is already completed meaning it doesn't matter much now, but to answer your question)
Couldn't this:
for (let i = 0; i < output.size; ++i) {
output = updateOutput(i, input, output, beginningPadding, mode, value);
}
Have been this?...
for (let i = 0; i < output.size; ++i) {
updateOutput(i, input, output, beginningPadding, mode, value);
}
...since updateOutput is already calling setValueByIndex on the destination.
[update]
Thanks: #50
|
@fdwr Thanks for your review, I'll address these comments after your complete review. |
src/pad.js
Outdated
| const sourceShape = source.shape; | ||
| const location = destination.locationFromIndex(index); | ||
| const rank = location.length; | ||
| let hasFillPadding = false; |
|
@huningxin Thanks for your reviewing. |
3 participants
@fdwr @huningxin PTAL, thanks.