Skip to content

Conversation

@BruceDai
Copy link
Contributor

@BruceDai BruceDai commented Dec 1, 2022

}
if (options.filterLayout) {
conv2dOptions.filterLayout = options.filterLayout;
}
Copy link

Choose a reason for hiding this comment

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

Too bad Javascript lacks a ??= operator.

      conv2dOptions.padding ??= options.padding;
      conv2dOptions.strides ??= options.strides;
      conv2dOptions.dilations ??= options.dilations;
      conv2dOptions.autoPad ??= options.autoPad;
      conv2dOptions.groups ??= options.groups;
      conv2dOptions.inputLayout ??= options.inputLayout;
      conv2dOptions.filterLayout ??= options.filterLayout;

Oh well. (no action expected, just musing)

Copy link
Contributor

Choose a reason for hiding this comment

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

unless I'm missing something, I think this could be accomplished with conv2dOptions = {...options};

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I've updated. Please take another look, thanks @dontcallmedom

Copy link

Choose a reason for hiding this comment

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

unless I'm missing something, I think this could be accomplished with conv2dOptions = {...options};

Cool. I remember COBOL had a "MOVE CORRESPONDING" statement which would copy all fields from source to destination of the same name. So it's neat to see an equivalent in newer languages.

}
if (options.inputLayout) {
if (!['nchw', 'nhwc'].includes(options.inputLayout)) {
throw new Error(`Unsupport inputLayout ${options.inputLayout}`);
Copy link

Choose a reason for hiding this comment

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

Unsupported

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Updated, thanks.

filterWidth = filterShape[2];
filterHeight = filterShape[1];
break;
default:
Copy link

Choose a reason for hiding this comment

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

I know you already read it above on lines 75 and 76, but it would be good to call it out here for clarity. e.g.

case 'oihw':
    // Just use the existing filterWidth and filterHeight above.
    break;

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 this 'oihw' case in switch block, thanks.

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.

That's a lot of work Bruce. You got all the primary cases. I'd just add one more with the combo of both dilations and strides that are greater than 1, because interesting bugs happen with the interaction of those two (at least, they have in the codebases I've worked on).

@BruceDai BruceDai force-pushed the add_webnn_conv2d_tests branch from 4b90f04 to c1a02b1 Compare December 5, 2022 07:11
@BruceDai
Copy link
Contributor Author

BruceDai commented Dec 5, 2022

add one more with the combo of both dilations and strides that are greater than 1

Added such one test, PTAL, thanks.

@BruceDai BruceDai force-pushed the add_webnn_conv2d_tests branch 2 times, most recently from ef9709a to 8d08d68 Compare December 5, 2022 08:09
@BruceDai BruceDai force-pushed the add_webnn_conv2d_tests branch from 8d08d68 to 9766cd7 Compare January 12, 2023 02:31
@Honry Honry merged commit c15ee82 into web-platform-tests:master Jan 12, 2023
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.

6 participants