completed l2pool2d #68
Conversation
src/reduce.js
Outdated
| @@ -143,6 +143,20 @@ export function reduceL1(input, options = {}) { | |||
| return reduceSum(abs(input), options); | |||
| } | |||
|
|
|||
| /* The l2 reducer */ | |||
| export function l2Reducer(previousValue, currentValue, currentIndex, array) { | |||
| if (currentIndex== 1) { | |||
src/reduce.js
Outdated
| /* The l2 reducer */ | ||
| export function l2Reducer(previousValue, currentValue, currentIndex, array) { | ||
| if (currentIndex== 1) { | ||
| const sumOfSquares = previousValue*previousValue + currentValue * currentValue; |
There was a problem hiding this comment.
[nit] ... = previousValue * previousValue ...
src/reduce.js
Outdated
| /* The l2 reducer */ | ||
| export function l2Reducer(previousValue, currentValue, currentIndex, array) { | ||
| if (currentIndex== 1) { | ||
| const sumOfSquares = previousValue*previousValue + currentValue * currentValue; |
There was a problem hiding this comment.
Hmm, if this is the first index, wouldn't the previousValue be ignorable?
const sumOfSquares = currentValue * currentValue;
Also, does this work correctly if there is only one element being reduced? Otherwise the final sqrt isn't applied. Let's be sure to include a test case with exactly one element @BruceDai. I would have expected:
if (currentIndex == 0)
previousValue = 0;
const sumOfSquares = previousValue + currentValue * currentValue;
return (currentIndex === array.length - 1) ? sumOfSquares : Math.sqrt(sumOfSquares);
There was a problem hiding this comment.
In this function, currentIndex starts from 1 by default, and the number with index=0 at the beginning defaults to previousValue. So I consider this situation: if (currentIndex== 1) {
const sumOfSquares = previousValue * previousValue + currentValue * currentValue;}
E.g.
case:
const x = new Tensor([1, 1, 4, 4], [1, 2, 3, 4, 5, 6, 7, 8, 9, 10, 11, 12, 13, 14, 15, 16]) ;
const windowDimensions = [3, 3];
log:
currentIndex: 1 previousValue: 1 currentValue: 2
currentIndex: 2 previousValue: 5 currentValue: 3
currentIndex: 3 previousValue: 14 currentValue: 5
currentIndex: 4 previousValue: 39 currentValue: 6
currentIndex: 5 previousValue: 75 currentValue: 7
currentIndex: 6 previousValue: 124 currentValue: 9
currentIndex: 7 previousValue: 205 currentValue: 10
currentIndex: 8 previousValue: 305 currentValue: 11
There was a problem hiding this comment.
currentIndex starts from 1 by default
Huh, why doesn't the first reduced element start at 0? Don't JS arrays start at 0?
Btw, just to catch any interesting boundary cases, I'd include one test where the values don't start at 1 (e.g. [1, 2, 3, 4, 5, 6, 7, 8, 9, 10, 11, 12, 13, 14, 15, 16] -> [16, 15, 14, 13, 12, 11, 10, 9, 8, 7, 6, 5, 4, 3, 2, 1]).
There was a problem hiding this comment.
| it('l2Pool2d nhwc', function() { | ||
| const x = new Tensor([1, 4, 4, 1], [1, 2, 3, 4, 5, 6, 7, 8, 9, 10, 11, 12, 13, 14, 15, 16]); | ||
| const windowDimensions = [3, 3]; | ||
| const layout = 'nhwc'; |
There was a problem hiding this comment.
(update) I found a use case for normalizing across batches and spatial dimensions - this may inform future decisions: https://ieeexplore.ieee.org/document/8648206
🤔 I've often wondered why pooling in ML doesn't have variations that can pool across batches or channel dimensions too, not only the "spatial" dimensions. e.g. One could generalize pooling consistently like reduction (which can apply from all dimensions to none), and pass windowDimensions of rank 4 with [3, 1, 3, 3] to pool along batches too, or just [1, 3, 1, 1] for only channels. Also it would eliminate the then redundant layout parameter. Now, DML can achieve the latter via explicit strides (to map the first dimension to the last dimensions), but there's no way to access it via WebNN, and I've never heard of a need for it anyway. 🤷 I suppose if a researcher really wanted it, they could transpose the batch dimension to the X dimension, pool, then transpose back; and if there has been such a use case, that's probably exactly what they did.
*No action expected - just musing after noticing this.
| 14, 15, 16, 17, 18, 19, 20, 21, 22, 23, 24, 25, | ||
| ]); | ||
| const windowDimensions = [5, 5]; | ||
| const autoPad = 'same-upper'; |
There was a problem hiding this comment.
Oh, we're still using autoPad for pooling.
@huningxin, you opened webmachinelearning/webnn#326 where we talked about removing autoPad from conv2d, and @Honry removed it from the WebNN EP in microsoft/onnxruntime#18688, but for consistency, wouldn't we want to be explicit with pooling too?
There was a problem hiding this comment.
but for consistency, wouldn't we want to be explicit with pooling too?
Yes, we should be consistent and only do explicit padding for pooling.
| 32.4037034920393, | ||
| ]; | ||
| utils.checkValue(y, expected); | ||
| }); |
There was a problem hiding this comment.
Shall we add a single element test case here (e.g. const x = new Tensor([1, 1, 1, 1], or wait for the WPT?
There was a problem hiding this comment.
Okay,I have added a single element test case.
@BruceDai @huningxin @fdwr PTAL , thanks！