Bugfix: Updated the step of calculating output sizes by scales for resample2d by BruceDai · Pull Request #611 · webmachinelearning/webnn

BruceDai · 2024-03-20T05:42:31Z

Preview | Diff

BruceDai · 2024-03-20T05:48:51Z

@huningxin @fdwr @lisa0314 PTAL, thanks.

index.bs

huningxin · 2024-03-20T13:43:45Z

index.bs

-        1. If |options|.{{MLResample2dOptions/sizes}} [=map/exists=], set |desc|'s [=MLOperand/shape=][|options|.{{MLResample2dOptions/axes}}[|index|]] to |options|.{{MLResample2dOptions/sizes}}[|index|] and return |desc|.
-        1. Otherwise, set |desc|'s [=MLOperand/shape=][|options|.{{MLResample2dOptions/axes}}[|index|]] to |input|'s [=MLOperand/shape=][|index|] multiplied by |options|.{{MLResample2dOptions/scales}}.
+        1. If |options|.{{MLResample2dOptions/sizes}} [=map/exists=], set |desc|.{{MLOperandDescriptor/dimensions}}[|options|.{{MLResample2dOptions/axes}}[|index|]] to |options|.{{MLResample2dOptions/sizes}}[|index|] and return |desc|.
+        1. Otherwise, set |desc|.{{MLOperandDescriptor/dimensions}}[|options|.{{MLResample2dOptions/axes}}[|index|]] to |input|'s [=MLOperand/shape=][|options|.{{MLResample2dOptions/axes}}[|index|]] multiplied by |options|.{{MLResample2dOptions/scales}}[|index|].


Should we use any one float to integer conversions (floor or ceil) for the result of input size multiplied by scale?

Just confirming: Setting |desc|.{{MLOperandDescriptor/dimensions}} looks correct to me.

I did some grepping and I don't see other instances in the spec of "blah's shape" where blah isn't an MLOperand.

Re: float-to-integer: Yes! The Chromium implementation uses floor() https://source.chromium.org/chromium/chromium/src/+/main:components/ml/webnn/graph_validation_utils.cc;l=986

Also this is mentioned in #360 so I think after this lands we can close that issue?

Suggested change

1. Otherwise, set |desc|.{{MLOperandDescriptor/dimensions}}[|options|.{{MLResample2dOptions/axes}}[|index|]] to |input|'s [=MLOperand/shape=][|options|.{{MLResample2dOptions/axes}}[|index|]] multiplied by |options|.{{MLResample2dOptions/scales}}[|index|].

1. Otherwise, set |desc|.{{MLOperandDescriptor/dimensions}}[|options|.{{MLResample2dOptions/axes}}[|index|]] to floor(|input|'s [=MLOperand/shape=][|options|.{{MLResample2dOptions/axes}}[|index|]] * |options|.{{MLResample2dOptions/scales}}[|index|]).

Yes, other resampling implementations also use floor too (which makes sense, because you wouldn't want 1.1 pixels to either yield empty space or repeat 1.1 pixels to 2 pixels of the same color).

Yeah, * is more concise than "multiplied by".

inexorabletash

Please add floor() to the calculation, otherwise LGTM.

BruceDai · 2024-03-21T01:26:16Z

I've updated commit to address comments. Please take another look, thanks.

inexorabletash

LGTM!

huningxin · 2024-03-21T05:02:35Z

index.bs

    1. For |index| in [=the range=] 0 to |options|.{{MLResample2dOptions/axes}}'s [=list/size=], exclusive:
-        1. If |options|.{{MLResample2dOptions/sizes}} [=map/exists=], set |desc|'s [=MLOperand/shape=][|options|.{{MLResample2dOptions/axes}}[|index|]] to |options|.{{MLResample2dOptions/sizes}}[|index|] and return |desc|.
-        1. Otherwise, set |desc|'s [=MLOperand/shape=][|options|.{{MLResample2dOptions/axes}}[|index|]] to |input|'s [=MLOperand/shape=][|index|] multiplied by |options|.{{MLResample2dOptions/scales}}.
+        1. If |options|.{{MLResample2dOptions/sizes}} [=map/exists=], set |desc|.{{MLOperandDescriptor/dimensions}}[|options|.{{MLResample2dOptions/axes}}[|index|]] to |options|.{{MLResample2dOptions/sizes}}[|index|] and return |desc|.


This step may not return |desc| in the loop. I understand the |desc| would be returned in the Line 4945.

Suggested change

1. If |options|.{{MLResample2dOptions/sizes}} [=map/exists=], set |desc|.{{MLOperandDescriptor/dimensions}}[|options|.{{MLResample2dOptions/axes}}[|index|]] to |options|.{{MLResample2dOptions/sizes}}[|index|] and return |desc|.

1. If |options|.{{MLResample2dOptions/sizes}} [=map/exists=], set |desc|.{{MLOperandDescriptor/dimensions}}[|options|.{{MLResample2dOptions/axes}}[|index|]] to |options|.{{MLResample2dOptions/sizes}}[|index|]

Thanks @huningxin . Please review this update in new commit.

huningxin

LGTM

…sample2d (#611) SHA: 24edf7f Reason: push, by fdwr Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>

Fixed the step of calculating output sizes by scales for resample2d

f8a59dc

BruceDai mentioned this pull request Mar 20, 2024

Need clarify scale factor for resample2d #610

Closed

BruceDai changed the title ~~Fixed the step of calculating output sizes by scales for resample2d~~ [Bugfix] Updated the step of calculating output sizes by scales for resample2d Mar 20, 2024

BruceDai changed the title ~~[Bugfix] Updated the step of calculating output sizes by scales for resample2d~~ Bugfix: Updated the step of calculating output sizes by scales for resample2d Mar 20, 2024

fdwr reviewed Mar 20, 2024

View reviewed changes

index.bs Show resolved Hide resolved

fdwr reviewed Mar 20, 2024

View reviewed changes

index.bs Outdated Show resolved Hide resolved

huningxin reviewed Mar 20, 2024

View reviewed changes

inexorabletash suggested changes Mar 20, 2024

View reviewed changes

Add floor() to the calculation for float-to-integer

1dc77eb

inexorabletash approved these changes Mar 21, 2024

View reviewed changes

huningxin reviewed Mar 21, 2024

View reviewed changes

Updated the step of calculating with sizes option

0c0816e

huningxin approved these changes Mar 21, 2024

View reviewed changes

fdwr approved these changes Mar 21, 2024

View reviewed changes

fdwr merged commit 24edf7f into webmachinelearning:main Mar 21, 2024

BruceDai mentioned this pull request Mar 28, 2024

Need to define the algorithm for the calculation of the output shape of Resample2d #360

Closed

	1. Otherwise, set \|desc\|.{{MLOperandDescriptor/dimensions}}[\|options\|.{{MLResample2dOptions/axes}}[\|index\|]] to \|input\|'s [=MLOperand/shape=][\|options\|.{{MLResample2dOptions/axes}}[\|index\|]] multiplied by \|options\|.{{MLResample2dOptions/scales}}[\|index\|].
	1. Otherwise, set \|desc\|.{{MLOperandDescriptor/dimensions}}[\|options\|.{{MLResample2dOptions/axes}}[\|index\|]] to floor(\|input\|'s [=MLOperand/shape=][\|options\|.{{MLResample2dOptions/axes}}[\|index\|]] * \|options\|.{{MLResample2dOptions/scales}}[\|index\|]).

	1. If \|options\|.{{MLResample2dOptions/sizes}} [=map/exists=], set \|desc\|.{{MLOperandDescriptor/dimensions}}[\|options\|.{{MLResample2dOptions/axes}}[\|index\|]] to \|options\|.{{MLResample2dOptions/sizes}}[\|index\|] and return \|desc\|.
	1. If \|options\|.{{MLResample2dOptions/sizes}} [=map/exists=], set \|desc\|.{{MLOperandDescriptor/dimensions}}[\|options\|.{{MLResample2dOptions/axes}}[\|index\|]] to \|options\|.{{MLResample2dOptions/sizes}}[\|index\|]

Comments

Conversation

BruceDai commented Mar 20, 2024 • edited by pr-preview bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

BruceDai commented Mar 20, 2024

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

fdwr Mar 21, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

inexorabletash left a comment

Choose a reason for hiding this comment

Uh oh!

BruceDai commented Mar 21, 2024

Uh oh!

inexorabletash left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

huningxin left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

BruceDai commented Mar 20, 2024 •

edited by pr-preview bot

Loading

fdwr Mar 21, 2024 •

edited

Loading