Add webnn reduce tests #37380
Add webnn reduce tests #37380
Conversation
| } | ||
| }, | ||
| "options": { | ||
| "axes": [-4, -2] |
There was a problem hiding this comment.
Higher level policy like negative axes in WebNN complicate implementations and testing :/ (and it's unnecessary since WebNN doesn't support arbitrary shape inference anyway). In future versions of WebNN, I'd like to see actual indices resolved by the higher level frameworks first.
| -45.433313937995614, | ||
| 20.781868403551314, | ||
| -66.96646315625907 | ||
| ], |
There was a problem hiding this comment.
You'll have to be careful summing mixed signs, due to subtraction of nearly equal numbers magnifying error. For reduceSum, I think it wiser to curate the input tensors to all positive values, all negative values, and/or all integers. Otherwise you'll chase down errors of catastrophic cancellation. The ULP values given are with this restriction in mind, as there are no reasonable tolerance values for nearly equal numbers - you either have to relax them (potentially greatly), which weakens the overall tolerance of all other cases, or you have to cross your fingers that you won't hit such a case (which is tough given the randomness of the values and the fact they can be added in any order depending on the implementation).
There was a problem hiding this comment.
Thanks. I will add such tests as your suggested.
There was a problem hiding this comment.
Done, added such tests for reduceMean and reduceSum ops, please have another look, thanks.
There was a problem hiding this comment.
Good that you added more test cases, but the existing problematic test cases of mixed signs in ReduceMean and ReduceSum remain and can encounter tolerance issues. So I was hoping those would be converted to consistent signs (or in light of the new test cases, be deleted).
"name": "reduceMean float32 4D tensor default options",
"inputs": {
"input": {
"shape": [2, 2, 2, 3],
"data": [
-1.913036787607325,
95.38300711992477,
-82.72358015843713,
-32.94943032326678,
91.42181191536278,
-75.1219094678544,
-70.81411336302938,
| } | ||
| }, | ||
| { | ||
| "name": "reduceSum float32 4D tensor negtive options.axes with options.keepDimensions=true", |
webnn/resources/utils.js
Outdated
| let tolerance = sizes.reduce( | ||
| (accumulator, currentValue) => accumulator * currentValue | ||
| ); | ||
| if (['reduceMean'].includes(operationName)) { |
There was a problem hiding this comment.
I don't know Javascript well, but is this better than just a string comparison? operationName == 'reduceMean' is clearer to me. 🤔
There was a problem hiding this comment.
Thanks.
I use this condition for extensibility, maybe other reduce operations need adopt this same tolerance later, so we just update array to solve this case, for example
if (['reduceMean', 'reduceXXXX'].includes(operationName)) There was a problem hiding this comment.
Ah. Putting the thing being tested after the things being tested against feels reversed :/. Alas, one would think you could readably say...
if (operationName in ['reduceMean', 'reduceXXXX'])
...but reading about JS's in operator, it behaves counterintuitively, returning true for things that are not in the list if an undefined value exists -_-. Oh well.
BruceDai
force-pushed
the
add_webnn_reduce_tests
branch
3 times, most recently
from
96672ce to
4b784fa
Compare
|
@fdwr Thanks for your reviewing, I've updated PR to address your comments. |
reduceMax/reduceMean/reduceMin/reduceProduct/reduceSum
BruceDai
force-pushed
the
add_webnn_reduce_tests
branch
from
ce4a53d to
0b25b16
Compare
|
@Honry All checks passed, PTAL, thanks. |
6 participants
@fdwr @Honry @dontcallmedom PTAL, thanks.