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

Use tensor type for the padding rather than MLOperand #224

Closed
mingmingtasd opened this issue Nov 8, 2021 · 5 comments
Closed

Use tensor type for the padding rather than MLOperand #224

mingmingtasd opened this issue Nov 8, 2021 · 5 comments

Comments

@mingmingtasd
Copy link
Contributor

The pad in the spec uses the MLOperand padding as its sencond input. But I find that the pad in DML and in IE, padding was defined with a tensor type. I think it's better to use tensor type to avoid the gap between spec and implementation.

@huningxin
Copy link
Contributor

huningxin commented Nov 23, 2021

As feedback of WebML WG Teleconference – 18 Nov 2021, the static attribute is preferred, e.g. @wchao1115 mentioned

+1 on prefer static, dynamism breaks accelerator's pipelining

Another reference is TensorFlow.js, according to tf.pad, the paddings is also an array.

With that, I propose to change the padding input of WebNN's pad to an array (sequence in WebIDL).

@mingmingtasd , I am not sure whether it aligns with your proposal. What do you mean by using "tensor type"?

@mingmingtasd
Copy link
Contributor Author

As feedback of WebML WG Teleconference – 18 Nov 2021, the static attribute is preferred, e.g. @wchao1115 mentioned

+1 on prefer static, dynamism breaks accelerator's pipelining

Another reference is TensorFlow.js, according to tf.pad, the paddings is also an array.

With that, I propose to change the padding input of WebNN's pad to an array (sequence in WebIDL).

@mingmingtasd , I am not sure whether it aligns with your proposal. What do you mean by using "tensor type"?

Yes, that aligns with what I mean: using array. Thanks! @huningxin

@huningxin
Copy link
Contributor

@mingmingtasd , as @wchao1115 's feedback, DirectML is supporting dynamic padding data. And NNAPI ANEURALNETWORKS_PAD_V2 also supports padding data as an operand. So it looks like the current pad op definition of WebNN is more future-proof.

With that, I would withdraw my proposal and keep the current version. WDYT?

@mingmingtasd
Copy link
Contributor Author

@mingmingtasd , as @wchao1115 's feedback, DirectML is supporting dynamic padding data. And NNAPI ANEURALNETWORKS_PAD_V2 also supports padding data as an operand. So it looks like the current pad op definition of WebNN is more future-proof.

With that, I would withdraw my proposal and keep the current version. WDYT?

Yes, I agree, thanks for your explaination. @huningxin

@huningxin
Copy link
Contributor

Thanks for the feedback @mingmingtasd ! I'll close PR #233 and this one.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants