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.

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 float32 tests for Element-wise logical operations #43304

Merged
merged 2 commits into from
Nov 28, 2023

Conversation

BruceDai
Copy link
Contributor

@fdwr @Honry PTAL, thanks.

@@ -277,6 +277,14 @@ const PrecisionMetrics = {
min: {ULP: {float32: 0, float16: 0}},
pow: {ULP: {float32: 32, float16: 2}},
// End Element-wise binary operations
// Begin Element-wise logical operations
equal: {ULP: {uint8: 0}},
Copy link
Contributor Author

@BruceDai BruceDai Nov 22, 2023

Choose a reason for hiding this comment

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

For logical operations, no mater input data type is float32 or float16 and etc. , the output type should be uint8.
And here uint8 is align with expected data type in json file, likes

"expected": {
  "name": "output",
  "shape": [2, 1, 4, 1, 3],
  "data": [
    0,
    0,
    0,
    1,
    0,
    ...
    0
  ],
  "type": "uint8"
}

We also use expected data type (precisionType) as a key to get tolerance:

// L#347
let tolerance = PrecisionMetrics[operationName][metricType][precisionType];

@fdwr Is it OK for this addition code? Thanks.

Copy link

Choose a reason for hiding this comment

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

👍

Copy link

@fdwr fdwr left a comment

Choose a reason for hiding this comment

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

👍

@@ -277,6 +277,14 @@ const PrecisionMetrics = {
min: {ULP: {float32: 0, float16: 0}},
pow: {ULP: {float32: 32, float16: 2}},
// End Element-wise binary operations
// Begin Element-wise logical operations
equal: {ULP: {uint8: 0}},
Copy link

Choose a reason for hiding this comment

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

👍

Copy link
Contributor

@Honry Honry left a comment

Choose a reason for hiding this comment

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

LGTM, thanks!

@BruceDai
Copy link
Contributor Author

Hi @fdwr, I synced with @huningxin, we decided to rename not op tests to previous logicalNot op tests, since Chromium WebNN API implementation has already supported logicalNot op, then these WPT logicalNot tests can be ran, please take another look at this update, thanks.

@fdwr
Copy link

fdwr commented Nov 28, 2023

@BruceDai : Ok, well whatever the final outcome of the spec is, it's a simple rename either way. 👍

@BruceDai
Copy link
Contributor Author

Rebase code to check whether last two fail CIs can pass, then go to ask for merging it.

@Honry Honry merged commit 138aaca into web-platform-tests:master Nov 28, 2023
19 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants