Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[webnn] Add tests for WebNN API constant (fillSequence) #43801

Merged
merged 4 commits into from
Feb 18, 2024
Merged

[webnn] Add tests for WebNN API constant (fillSequence) #43801

Changes from 1 commit
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
65b65d5
[webnn] Add tests for WebNN API constant (fillSequence)
BruceDai Dec 27, 2023
0f98ac8
[webnn] Update test data with negative start
BruceDai Jan 18, 2024
8d8e82b
[webnn] Update test data for integer test cases
BruceDai Jan 19, 2024
ccd5945
[webnn] Add a test with negative integer step
BruceDai Jan 22, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
24 changes: 12 additions & 12 deletions webnn/resources/test_data/constant.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -616,7 +616,7 @@
"name": "constant float32 4D tensor of int8 type",
"inputs": {
"start": {
"data": 0.22992068529129028,
"data": -5.520709991455078,
"type": "float32"
Copy link

@fdwr fdwr Jan 18, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Wait, this isn't supported, mixing and matching different data types like float32 and int8. If the output is an integer tensor, then the start and delta must be integers too. It would be legal to pass start of -5 and delta of 1 though. Otherwise DirectML will complain (fail API validation) that the DML_FILL_VALUE_SEQUENCE_OPERATOR_DESC::ValueDataType does not match the outputTensorDesc->Type.

Bin Miao (https://chromium-review.googlesource.com/c/chromium/src/+/5091540) is currently working on a way via the IDL to express both float64 and int64 in the API.

Also by mixing types, you'll get weird artifacts like this repeated 0 x 3 in the output data:

          -2,
          -1,
          0, // <-----
          0, // <-----
          0, // <-----
          1,
          2,

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So we need use integer numbers for such cases, though Spec define start and step as float, or does it need update this Spec?

start: a [float](https://webidl.spec.whatwg.org/#idl-float) scalar. The starting value of the range.

step: a [float](https://webidl.spec.whatwg.org/#idl-float) scalar. The gap value between two data points in the range.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, we should update the spec with additional wording. Even if you pass it as a float/double, internally it will be casted to an integer before any calculations are done with it. So then the baseline should cast any floating-point values (start and delta values) to integers (or BigInt) before computing.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For now, for the integer cases, can we just pass integer start and delta values?
"data": -5.520709991455078, -> "data": -5,

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sure, I'll update test data for those integer cases.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done, please have another look, thanks.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

One case with a negative step for the integers would be good, but LGTM.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Added. Please take another look, thanks.

},
"step": {
Expand All @@ -630,30 +630,30 @@
"name": "output",
"shape": [2, 2, 2, 3],
"data": [
-5,
-4,
-4,
-3,
-2,
-1,
0,
0,
0,
1,
2,
3,
2,
3,
4,
5,
5,
6,
7,
7,
8,
8,
9,
10,
10,
11,
12,
13,
13,
14,
15,
16,
16,
17
11
],
"type": "int8"
}
Expand Down