-
Notifications
You must be signed in to change notification settings - Fork 3.6k
Add webnn conv2d tests #37275
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
Add webnn conv2d tests #37275
Conversation
webnn/conv2d.https.any.js
Outdated
| } | ||
| if (options.filterLayout) { | ||
| conv2dOptions.filterLayout = options.filterLayout; | ||
| } |
There was a problem hiding this comment.
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)
There was a problem hiding this comment.
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};
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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.
webnn/resources/utils.js
Outdated
| } | ||
| if (options.inputLayout) { | ||
| if (!['nchw', 'nhwc'].includes(options.inputLayout)) { | ||
| throw new Error(`Unsupport inputLayout ${options.inputLayout}`); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Unsupported
There was a problem hiding this comment.
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: |
There was a problem hiding this comment.
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;
There was a problem hiding this comment.
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.
fdwr
left a comment
There was a problem hiding this 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).
4b90f04 to
c1a02b1
Compare
Added such one test, PTAL, thanks. |
ef9709a to
8d08d68
Compare
8d08d68 to
9766cd7
Compare
@fdwr @Honry @huningxin @dontcallmedom PTAL, thanks.