Skip to content

Comments

Use unsigned integer axis#359

Merged
huningxin merged 4 commits intowebmachinelearning:mainfrom
huningxin:unsigned_axis
Mar 8, 2023
Merged

Use unsigned integer axis#359
huningxin merged 4 commits intowebmachinelearning:mainfrom
huningxin:unsigned_axis

Conversation

@huningxin
Copy link
Contributor

@huningxin huningxin commented Mar 3, 2023 •
edited by pr-preview bot
Loading

fix #345, #317

This PR makes the axis/axes definitions consistent by using the unsigned integer type and valid value range [0, N-1] where N is the tensor rank across the spec.

@wchao1115 @wacky6 @fdwr @anssiko @pyu10055 @RafaelCintron , PTAL. Thanks!

Preview | Diff

@huningxin
Use unsigned long type for axis
e28ec4c 
fix webmachinelearning#345
@huningxin
Use range [0, N-1]
56ed80e 
It also fixes the batchNorm default axis value.
@huningxin huningxin mentioned this pull request Mar 3, 2023
Closed
anssiko
anssiko approved these changes Mar 3, 2023
View reviewed changes
Copy link
Member

@anssiko anssiko left a comment

Choose a reason for hiding this comment

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

🚀

@huningxin huningxin mentioned this pull request Mar 3, 2023
Closed
@fdwr
Copy link
Collaborator

fdwr commented Mar 4, 2023

@huningxin : Should we split the MLBatchNormalizationOptions default into a separate PR for clarity from the signed -> unsigned aspect?

fdwr
fdwr approved these changes Mar 4, 2023
View reviewed changes
Copy link
Collaborator

@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.

Thanks. Minor proposals, but I'm also content as-is. Rescanning the spec, it appears you got all the "long axis" and "sequence" occurrences. 👍

@huningxin
Address Dwayne's review comments
ef94c4e 
1. Leave the fix of webmachinelearning#307 to a separate PR
2. Refine MLTransposeOptions.permutation and MLSliceOptions.axes with examples
@huningxin
Copy link
Contributor Author

huningxin commented Mar 4, 2023 •
edited
Loading

@fdwr

@huningxin : Should we split the MLBatchNormalizationOptions default into a separate PR for clarity from the signed -> unsigned aspect?

sgtm. I removed the change of MLBatchNormalizationOptions.axis default value in the latest commit. Please take another look. Thanks!

fdwr
fdwr approved these changes Mar 4, 2023
View reviewed changes
Copy link
Collaborator

@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.

LGTM. TY.

@huningxin
Copy link
Contributor Author

huningxin commented Mar 8, 2023

Thanks for the review and approvals. With that, I am going to merge this PR.

@huningxin huningxin merged commit 2e302c9 into webmachinelearning:main Mar 8, 2023
github-actions bot added a commit that referenced this pull request Mar 8, 2023
@huningxin @github-actions
Merge pull request #359 from huningxin/unsigned_axis
c6376ff 
SHA: 2e302c9
Reason: push, by huningxin

Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
@BruceDai BruceDai mentioned this pull request Mar 8, 2023
Closed
aarongable pushed a commit to chromium/chromium that referenced this pull request Mar 10, 2023
@miaobin
WebNN: Fix axis of concat MLOperator according to WebNN spec
3599a69 
This CL implements the WebNN spec change [1] that changes axis of concat
MLOperator to be an unsigned integer.

The MLGraphBuilder and MLGraphBuilderTest are also updated according to
this IDL change.

[1]: webmachinelearning/webnn#359

Bug: 1273291
Change-Id: I519a070e6b2fed78cecc736004895c67bda9c785
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/4328150
Commit-Queue: Bin Miao <[email protected]>
Reviewed-by: Jiewei Qian <[email protected]>
Cr-Commit-Position: refs/heads/main@{#1115579}
aarongable pushed a commit to chromium/chromium that referenced this pull request Mar 15, 2023
@lisa0314
WebNN: Use unsigned integer axis for resample2d and transpose
c913067 
This CL implements the WebNN spec change [1] that changes axis of both
resample2d MLOperator and transpose MLOperator to be sequence<unsigned
long>.

The MLGraphBuilderTests are also updated according to this IDL change.

[1]: webmachinelearning/webnn#359

Bug: 1273291
Change-Id: I1bfbc431481ee5ee55022b88cf77ba8436d25319
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/4331801
Reviewed-by: Jiewei Qian <[email protected]>
Commit-Queue: Lisha Guo <[email protected]>
Cr-Commit-Position: refs/heads/main@{#1117357}
@BruceDai BruceDai mentioned this pull request Mar 16, 2023
Merged
@fdwr
Copy link
Collaborator

fdwr commented Mar 27, 2023

@huningxin : I should have noticed earlier while reviewing, but I just noticed slices parameter starts still has the negative number policy and accepts axis coordinates as long rather than unsigned long; and its sizes parameter has an unusual policy that -1 is special, whereas sizes in resample2D is sequence<unsigned long>. Do we want to rationalize these for consistency too, having higher level policy like this determined by the calling framework?

@huningxin
Copy link
Contributor Author

huningxin commented Mar 27, 2023

Thanks @fdwr . This PR focused on the axis/axes, so it just changed MLSliceOptions.axes to sequence<unsigned long>. For starts, because it is actually the offset along each dimension, it was left as-is. There was similar reason for unchanged sizes.

However, I agree with you that using unsigned long for starts and sizes may help the API consistency and simplify the implementation. The slice op of native ML APIs, like DirectML and XNNPACK, seem to only support unsigned starts and sizes. Another consideration would be whether we should simplify the pad by removing MLSliceOptions.axes. It can just require the length of starts and sizes is same as input tensor's rank.

I propose to open a separate issue to track it, WDYT?

@fdwr
Copy link
Collaborator

fdwr commented Mar 27, 2023 •
edited
Loading

@huningxin : 👍 Separate issue - thanks.

may help the API consistency and simplify the implementation ...

Yeah, we see that every other sizes parameter is unsigned long (see below), leaving only slice as sequence<long> sizes.

dictionary MLResample2dOptions {
...
  sequence<unsigned long> sizes;
...
};

dictionary MLPool2dOptions {
  ...
  sequence<unsigned long> outputSizes;
};

dictionary MLPool2dOptions {
...
  sequence<unsigned long> outputSizes;
};

Another consideration would be whether we should simplify the pad by removing MLSliceOptions.axes. It can just require the length of starts and sizes is same as input tensor's rank.

🤔⏳ Not sure, as I want to know what XNNPack does first too (DML_SLICE1_OPERATOR_DESC expects all dimensions to be listed explicitly, and unchanged dimensions just use the full range), but I generally lean the direction of being more explicit for WebNN.

@huningxin
Copy link
Contributor Author

huningxin commented Mar 27, 2023

@fdwr

as I want to know what XNNPack does first too

xnn_define_static_slice also expects all dimensions to be listed.

but I generally lean the direction of being more explicit for WebNN.

+1

@huningxin huningxin mentioned this pull request Mar 28, 2023
Closed
@inexorabletash inexorabletash mentioned this pull request Jan 29, 2024
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@anssiko anssiko anssiko approved these changes

@fdwr fdwr fdwr approved these changes

@wchao1115 wchao1115 wchao1115 approved these changes

@pyu10055 pyu10055 Awaiting requested review from pyu10055

@RafaelCintron RafaelCintron Awaiting requested review from RafaelCintron

+1 more reviewer

@wacky6 wacky6 wacky6 approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Use unsigned long for axis of concat operation

5 participants

@huningxin @fdwr @anssiko @wchao1115 @wacky6