Implement resample2d #33
Conversation
src/resample2d.js
Outdated
| * @return {Tensor} | ||
| */ | ||
| function nearestNeighbor(src, dst, dstHeight, dstWidth, scaleHeight, scaleWidth) { | ||
| const [srcBatches, srcHeight, srcWidth, srcChannels] = src.shape; |
There was a problem hiding this comment.
Curiously your for loop below is NHWC, but the shape is NCHW. It would be slightly more performant to change the shape to NCHW like the loop, so they have consistent memory access patterns, not that perf really matters in the test code, but it is nice to have the two coherent when reading it.
There was a problem hiding this comment.
Good catch! Now I let them be the same of NCHW, thanks.
src/resample2d.js
Outdated
| ih = 0; | ||
| } else if (ih > srcHeight - 1) { | ||
| ih = srcHeight - 1; | ||
| } |
There was a problem hiding this comment.
How about just ih = Math.max(Math.min(ih, srcHeight - 1), 0);?
I'd say just use Math.clamp(), but it seems the Math.clamp proposal never entered Javascript .
src/resample2d.js
Outdated
| scales = [1.0, 1.0], | ||
| sizes, | ||
| axes = [2, 3], | ||
| } = {}) { |
There was a problem hiding this comment.
(minor) It's kinda hard to follow these } = {}) {. Line break somewhere in there to aid readability?
src/resample2d.js
Outdated
| } = {}) { | ||
| if (axes[0] === 0) { | ||
| // hwnc -> nhwc | ||
| input = transpose(input, {permutation: [2, 0, 1, 3]}); |
There was a problem hiding this comment.
It would be more robust/clearer to verify that axes[0] == 0 && axes[1] == 1, rather than just axes[0] == 0.
I'd also add an else for axes[0] == 1 && axes[1] == 2 for clarity, even if it is just a comment that says "NHWC -> NHWC Do nothing". That way it's apparent to readers that you didn't leave it out.
There was a problem hiding this comment.
Btw, I had this clever idea that you could just say a single line...
input = transpose(permutation: [(axes[0] + 2) & 3, (axes[1] + 2) & 3, axes[0], axes[1]]);
...instead of explicit switch case statements for each transpose, and it works nicely and generically for the first transpose (it assigns the last 2 dimensions H and W to read from whatever axes were given, and it assigns the batch and channel to the other two axes). All the standard axes work:
axes = [0,1] -> permute:[2, 3, 0, 1]
axes = [1,2] -> permute:[3, 0, 1, 2]
axes = [2,3] -> permute:[0, 1, 2, 3]
And it works for the mirrors of the standard ones:
axes = [1,0] -> permute:[3, 2, 1, 0]
axes = [2,1] -> permute:[0, 3, 2, 1]
axes = [3,2] -> permute:[1, 0, 3, 2]
And even works for HNCW (which isn't valid per the spec anyway):
axes = [0,3] -> permute:[2, 1, 0, 3]
But it's not clear how to untranspose that (the 2nd transpose). So, I'm not proposing that idea... 🤔🤷♂️
There was a problem hiding this comment.
input = transpose(permutation: [(axes[0] + 2) & 3, (axes[1] + 2) & 3, axes[0], axes[1]]);'
Thanks, this code is fairly concise.
For more easily understand, I modified previous switch case statements following your first comment.
| data: [ | ||
| 1, 1, 1, 2, 2, 2, 1, 1, 1, 2, 2, 2, | ||
| 3, 3, 3, 4, 4, 4, 3, 3, 3, 4, 4, 4, | ||
| ], |
There was a problem hiding this comment.
The output would be clearer if wrapped consistently with the shape. e.g.
data: [
1, 1, 1, 2, 2, 2,
1, 1, 1, 2, 2, 2,
3, 3, 3, 4, 4, 4,
3, 3, 3, 4, 4, 4,
],
Like you nicely did with the other cases above. e.g.
shape: [1, 1, 4, 4],
data: [
1.0, 1.25, 1.75, 2.0,
1.5, 1.75, 2.25, 2.5,
2.5, 2.75, 3.25, 3.5,
3.0, 3.25, 3.75, 4.0,
],
There was a problem hiding this comment.
Updated test data, thanks.
|
@fdwr Thanks for your reviewing! I updated commit to address your comments, please take another look, thanks. |
@fdwr Thanks much for explanations of resample2d by
nearest-neighborandlinearmodes on #18 and webmachinelearning/webnn#358, this PR is to sample pixel centers to align with WebNN-Polyfill and Chromium WebNN implementation.@fdwr @huningxin @lisa0314 PTAL, thanks.