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.

Sign up for GitHub

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

Jump to bottom

Reserve asserts only for non-obvious things #455

Closed
zolkis opened this issue Aug 23, 2023 · 4 comments
Closed

Reserve asserts only for non-obvious things #455

zolkis opened this issue Aug 23, 2023 · 4 comments
Assignees
@zolkis
Labels
conventions

Comments

@zolkis
Copy link
Collaborator

zolkis commented Aug 23, 2023 •
edited
Loading

Originally posted by @inexorabletash in #446 (comment)

FWIW, I'd reserve asserts only for non-obvious things, e.g. where a much earlier step in an algorithm has ensured that a number is non-zero, so a division by it is safe.
Asserts like these that repeat the concise Web IDL definition in longer prose are just noise and IMHO not useful for implementers.
@zolkis
Copy link
Collaborator Author

zolkis commented Aug 23, 2023

The difficulty is that e.g. the "create..." and "validate..." type of internal steps have no WebIDL, so asserts make sense there IMHO, but indeed in the algorithms with WebIDL definitions they can be removed.
@anssiko suggested we do this after #446 is merged.

@zolkis zolkis changed the title FWIW, I'd reserve asserts only for non-obvious things, e.g. where a much earlier step in an algorithm has ensured that a number is non-zero, so a division by it is safe. Reserve asserts only for non-obvious things Aug 23, 2023
@zolkis zolkis mentioned this issue Aug 23, 2023
Merged
@anssiko anssiko mentioned this issue Aug 24, 2023
Closed
@zolkis zolkis self-assigned this Aug 25, 2023
@inexorabletash inexorabletash mentioned this issue Nov 7, 2023
Merged
inexorabletash added a commit to inexorabletash/webnn that referenced this issue Dec 19, 2023
@inexorabletash
Spec Conventions: Tidy up "create an MLActivation" internal steps
5627f5e 
* Update callers to use dfn autolinks (for type checking)
* Remove assertion about builder argument (simplify)
* Remove test/exception for name argument (not needed)

For webmachinelearning#450 and webmachinelearning#455
inexorabletash added a commit to inexorabletash/webnn that referenced this issue Dec 19, 2023
@inexorabletash
Conventions: Use Infra style for iterating MLNamedArrayBufferViews
44e642e 
* Use "for each key → value of ..." syntax
* Drop asserts for types

For webmachinelearning#450 and webmachinelearning#455
inexorabletash added a commit to inexorabletash/webnn that referenced this issue Dec 19, 2023
@inexorabletash
Conventions: Tidy up "create an MLActivation" internal steps
0135f43 
* Update callers to use dfn autolinks (for type checking)
* Remove assertion about builder argument (simplify)
* Remove test/exception for name argument (not needed)

For webmachinelearning#450 and webmachinelearning#455
inexorabletash added a commit to inexorabletash/webnn that referenced this issue Dec 19, 2023
@inexorabletash
Conventions: Use Infra style for iterating MLNamedArrayBufferViews
74b87c6 
* Use "for each key → value of ..." syntax
* Drop asserts for types

For webmachinelearning#450 and webmachinelearning#455
@inexorabletash inexorabletash mentioned this issue Jan 5, 2024
Closed
@inexorabletash
Copy link
Member

FYI I have a branch ready for this, but #514 needs to merge first.

@anssiko
Copy link
Member

anssiko commented Jan 18, 2024

#514 is now merged.

inexorabletash added a commit to inexorabletash/webnn that referenced this issue Jan 18, 2024
@inexorabletash
Wording change: Drop arg type assertions for methods defined by WebIDL
05d4982 
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
inexorabletash added a commit to inexorabletash/webnn that referenced this issue Jan 18, 2024
@inexorabletash
Wording change: Convert algorithm type assertions into parameter types
df26d22 
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
@inexorabletash inexorabletash mentioned this issue Jan 18, 2024
Merged
inexorabletash added a commit to inexorabletash/webnn that referenced this issue Jan 21, 2024
@inexorabletash
Wording change: Drop arg type assertions for methods defined by WebIDL
b4a9359 
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
inexorabletash added a commit to inexorabletash/webnn that referenced this issue Jan 21, 2024
@inexorabletash
Wording change: Convert algorithm type assertions into parameter types
252631b 
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 pushed a commit that referenced this issue Jan 27, 2024
@inexorabletash
Wording change: Don't use asserts for validating argument types (#518)
5c3e2f9 
* 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 #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 #455
inexorabletash added a commit to inexorabletash/webnn that referenced this issue Jan 30, 2024
@inexorabletash
Wording change: Don't use asserts for validating argument types (webm…
2e1ebe3 
…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
@inexorabletash
Copy link
Member

The remaining asserts are either:

... so I'm going to go ahead and close this.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@zolkis zolkis

Labels
conventions
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
3 participants
@anssiko @inexorabletash @zolkis