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

Need clarify scale factor for resample2d #610

Closed
BruceDai opened this issue Mar 20, 2024 · 10 comments · Fixed by #641
Closed

Need clarify scale factor for resample2d #610

BruceDai opened this issue Mar 20, 2024 · 10 comments · Fixed by #641
Assignees
Labels

Comments

@BruceDai
Copy link
Contributor

BruceDai commented Mar 20, 2024

Link to resample2d definitions: https://webmachinelearning.github.io/webnn/#mlgraphbuilder-calculate-resample-output-sizes.

  1. The output sizes by calculating with scales are not correct, and the desc's shape is not accurate since the shape is defined as "An MLOperand's shape is its [[descriptor]].dimensions.". - I submitted a pr Bugfix: Updated the step of calculating output sizes by scales for resample2d #611 to fix it, please take a review, thanks.

  2. And it needs clarify the limitations for the product of scale factor and spatial dimension's size, I write such tests in my CL-5382118, please take a look, thanks.

@inexorabletash
Copy link
Member

inexorabletash commented Mar 21, 2024

1 seems addressed by the PR / 24edf7f

The spec doesn't generally handle overflow right now. A more general issue and a convention for handling that would be good.

@inexorabletash
Copy link
Member

Re: overflow - see #582 (comment) for some discussion

@BruceDai
Copy link
Contributor Author

And the scale value should be positive value which was raised at CL-5382118 reviewing.

@huningxin

The current spec doesn't put any restrictions on scale value, however the implementation 1 requests it to be positive value.

@huningxin
Copy link
Contributor

The "check resample options" step check the scale value should be positive:

If options.scales does not exist, set it to the list « 1.0, 1.0 ».
Otherwise, if any of its values is not greater than 0, or if its size is not 2, return false.

@BruceDai BruceDai changed the title The step "calculate resample output sizes" for resample2d has an issue and need some clarifications for scale factor Need clarify scale factor for resample2d Mar 28, 2024
@inexorabletash
Copy link
Member

Side observation: the Chromium code has if (scales[0] < 0 || scales[1] < 0) - it seems like those tests should be <= instead

https://source.chromium.org/chromium/chromium/src/+/main:components/ml/webnn/graph_validation_utils.cc;l=1024

@huningxin
Copy link
Contributor

Side observation: the Chromium code has if (scales[0] < 0 || scales[1] < 0) - it seems like those tests should be <= instead

+1

https://source.chromium.org/chromium/chromium/src/+/main:components/ml/webnn/graph_validation_utils.cc;l=1024

@mingmingtasd, could you please take a look at the chromium impl?

@mingmingtasd
Copy link
Contributor

Side observation: the Chromium code has if (scales[0] < 0 || scales[1] < 0) - it seems like those tests should be <= instead

https://source.chromium.org/chromium/chromium/src/+/main:components/ml/webnn/graph_validation_utils.cc;l=1024

Thanks! Good catch! I open: https://issues.chromium.org/issues/332587370 to track, and will fix that.

@inexorabletash
Copy link
Member

Re: clarify the limitations for the product of scale factor and spatial dimension's size

I filed #636 for the general issue. One thing I'm not clear on reading the code (https://source.chromium.org/chromium/chromium/src/+/main:components/ml/webnn/graph_validation_utils.cc;l=974 I think) is: is this just an implementation imposed limit because the Chromium implementation happens to use uint32_t internally?

@huningxin
Copy link
Contributor

@inexorabletash

One thing I'm not clear on reading the code (https://source.chromium.org/chromium/chromium/src/+/main:components/ml/webnn/graph_validation_utils.cc;l=974 I think) is: is this just an implementation imposed limit because the Chromium implementation happens to use uint32_t internally?

I suppose this is not an implementation imposed limit. According to calculate resample output sizes step

Otherwise, set desc.dimensions[options.axes[index]] to floor(input’s shape[options.axes[index]] * options.scales[index]).

Because desc.dimensions[options.axes[index]] is of type uint32, if the result of floor(input’s shape[options.axes[index]] * options.scales[index]) is out of range of uint32, it should throw a TypeError. I guess the value 0 limit is due to WebNN doesn't allow 0 size dimension that is being discussed at #391.

@inexorabletash
Copy link
Member

Because desc.dimensions[options.axes[index]] is of type uint32, if the result of floor(input’s shape[options.axes[index]] * options.scales[index]) is out of range of uint32, it should throw a TypeError.

Heh, thanks for reading the spec for me. :) So we should spec that. I'll put up a PR.

@inexorabletash inexorabletash self-assigned this Apr 8, 2024
inexorabletash added a commit to inexorabletash/webnn that referenced this issue Apr 8, 2024
If explicit sizes aren't provided, the size of an output dimension is
calculated as shape[i] * scales[i] which could be larger than can be
represented as a dimension in MLOperandDescriptor.

Explicitly validate and throw if this constraint isn't satisfied.

Fixes webmachinelearning#610
@fdwr fdwr closed this as completed in #641 Apr 16, 2024
fdwr added a commit that referenced this issue Apr 16, 2024
…shapes (#641)

* resample2d: Validate that dimensions are valid unsigned longs

If explicit sizes aren't provided, the size of an output dimension is
calculated as shape[i] * scales[i] which could be larger than can be
represented as a dimension in MLOperandDescriptor.

Explicitly validate and throw if this constraint isn't satisfied.

Fixes #610

* Introduce 'valid dimension' term

* Validate calculated dimensions when determining output shapes

* Update index.bs

Co-authored-by: Ningxin Hu <[email protected]>

* Update index.bs

Co-authored-by: Dwayne Robinson <[email protected]>

---------

Co-authored-by: Ningxin Hu <[email protected]>
Co-authored-by: Dwayne Robinson <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants