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

Wording change: Don't use asserts for validating argument types #518

Merged

Conversation

inexorabletash
Copy link
Member

@inexorabletash inexorabletash commented Jan 18, 2024

This PR has two changes, and is submitted as two commits for ease of review, although it's a single "theme" so one PR:

  1. Drop arg type assertions for methods defined by WebIDL.
  2. Convert internal algorithm type assertions into parameter types, following Infra conventions

For #455


Preview | Diff

index.bs Show resolved Hide resolved
@zolkis
Copy link
Collaborator

zolkis commented Jan 18, 2024

On first quick look this seems incomplete.
For instance, consistently replace |input| with {{MLOperand}} |input| (and similarly with other args)?

@inexorabletash
Copy link
Member Author

inexorabletash commented Jan 18, 2024

For instance, consistently replace |input| with {{MLOperand}} |input| (and similarly with other args)?

For algorithms that are specifying a method declared by WebIDL that's not typically done; the WebIDL declaration is considered sufficient to specify the types. From WebIDL:

The method steps of an operation operation should be introduced using text of the form “The operation(arg1, arg2, ...) method steps are:” followed by a list, or “The operation(arg1, arg2, ...) method steps are to” followed by an inline description.

That's different than how internal algorithms are declared, where there is no other source defining the types. From Infra:

Declare algorithms by stating their name, parameters, and return type, in the following form:

To [algorithm name], given a [type1] [parameter1], a [type2] [parameter2], …, perform the following steps. They return a [return type].

As noted in one of the commit comments, this PR doesn't go in and define all parameter types for all internal algorithms, just where there had previously been asserts. It would be good follow-up work to do so, but I wanted to keep the PR focused.

These don't improve readability of the spec.

To keep this commit focused, it does not touch similar assertions
for internal algorithms.

(Also, convert one NOTE into an ISSUE since it represents pending
spec work.)

For webmachinelearning#455
Improve internal algorithm readability by including the type of
parameters in the declaration of the algorithm, rather than as
assertions.

This commit just touches the algorithms with assertions. Other
algorithms could be improved with parameter type declarations, but
that'll be a follow-up.

For webmachinelearning#455
@wchao1115
Copy link
Collaborator

@inexorabletash Thanks for the change. This is another "best practice" change that ideally we should put into a convention guidelines doc along with the ID naming convention in another PR, so that the spec can be consistent after this change going forward.

inexorabletash added a commit to inexorabletash/webnn that referenced this pull request Jan 21, 2024
As suggested by @wchao1115 in webmachinelearning#499 and webmachinelearning#518, create a doc capturing
past WG disscussions in the telecons, issues, and PRs, so that we can
be consistent going forward.
inexorabletash added a commit to inexorabletash/webnn that referenced this pull request Jan 21, 2024
As suggested by @wchao1115 in webmachinelearning#499 and webmachinelearning#518, create a doc capturing
past WG disscussions in the telecons, issues, and PRs, so that we can
be consistent going forward.
inexorabletash added a commit to inexorabletash/webnn that referenced this pull request Jan 22, 2024
As suggested by @wchao1115 in webmachinelearning#499 and webmachinelearning#518, create a doc capturing
past WG disscussions in the telecons, issues, and PRs, so that we can
be consistent going forward.
inexorabletash added a commit to inexorabletash/webnn that referenced this pull request Jan 22, 2024
As suggested by @wchao1115 in webmachinelearning#499 and webmachinelearning#518, create a doc capturing
past WG disscussions in the telecons, issues, and PRs, so that we can
be consistent going forward.
anssiko pushed a commit that referenced this pull request Jan 24, 2024
…525)

As suggested by @wchao1115 in #499 and #518, create a doc capturing
past WG disscussions in the telecons, issues, and PRs, so that we can
be consistent going forward.
@inexorabletash
Copy link
Member Author

For completeness: text about asserts and algorithm declarations/definitions was included in the initial landing of SpecCodingConventions.md so this should be good to go.

@wchao1115 wchao1115 merged commit 5c3e2f9 into webmachinelearning:main Jan 27, 2024
2 checks passed
github-actions bot added a commit that referenced this pull request Jan 27, 2024
SHA: 5c3e2f9
Reason: push, by wchao1115

Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
@inexorabletash inexorabletash deleted the wording-assert-rework branch January 27, 2024 18:50
inexorabletash added a commit to inexorabletash/webnn that referenced this pull request Jan 30, 2024
…achinelearning#518)

* Wording change: Drop arg type assertions for methods defined by WebIDL

These don't improve readability of the spec.

To keep this commit focused, it does not touch similar assertions
for internal algorithms.

(Also, convert one NOTE into an ISSUE since it represents pending
spec work.)

For webmachinelearning#455

* Wording change: Convert algorithm type assertions into parameter types

Improve internal algorithm readability by including the type of
parameters in the declaration of the algorithm, rather than as
assertions.

This commit just touches the algorithms with assertions. Other
algorithms could be improved with parameter type declarations, but
that'll be a follow-up.

For webmachinelearning#455
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.

4 participants