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

Improve reviewability of WebNN operations baseline #1

Merged
merged 7 commits into from
Feb 9, 2022

Conversation

dontcallmedom
Copy link

No description provided.

Copy link
Owner

@huningxin huningxin left a comment

Choose a reason for hiding this comment

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

Thanks much @dontcallmedom !

Lint tool reports a few errors. Please take a look. You can run the lint tool locally by npm run lint. If you found lint rules are inappropriate, feel free to update the rules accordingly.

src/batch_normalization.js Outdated Show resolved Hide resolved
src/gru.js Outdated Show resolved Hide resolved
test/batch_normalization_test.js Outdated Show resolved Hide resolved
src/relu.js Outdated Show resolved Hide resolved
}
}

export function validateInput(op, args) {
Copy link
Owner

@huningxin huningxin Feb 7, 2022

Choose a reason for hiding this comment

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

Should we have specific method (or even separate file) for each op's validation? Say, validateConv2dInput etc.,

Copy link
Author

Choose a reason for hiding this comment

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

if we keep input validation, and if we keep it separate, separate methods are probably indeed much cleaner; I'm not sure separate files are necessary warranted, but I don't mind strongly either

Copy link
Owner

Choose a reason for hiding this comment

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

Regarding to discussion, it looks like we agree to keep the input validation code. So could you please make separate methods for them? I agree with you we can skip the separate files.

Copy link
Owner

@huningxin huningxin left a comment

Choose a reason for hiding this comment

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

Thanks @dontcallmedom for the update. My only open is about separate methods for input validation. Other code LGTM.

}
}

export function validateInput(op, args) {
Copy link
Owner

Choose a reason for hiding this comment

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

Regarding to discussion, it looks like we agree to keep the input validation code. So could you please make separate methods for them? I agree with you we can skip the separate files.

Copy link
Owner

@huningxin huningxin left a comment

Choose a reason for hiding this comment

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

LGTM, great work, thanks much @dontcallmedom !

@huningxin huningxin merged commit 6d81532 into huningxin:main Feb 9, 2022
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 this pull request may close these issues.

2 participants