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

Define algorithms for dictionaries with lists as default values #211

Closed
anssiko opened this issue Sep 9, 2021 · 5 comments
Closed

Define algorithms for dictionaries with lists as default values #211

anssiko opened this issue Sep 9, 2021 · 5 comments

Comments

@anssiko
Copy link
Member

anssiko commented Sep 9, 2021

Via @domenic https://github.com/webmachinelearning/webnn/pull/204/files#r703625252

You need to have a normative algorithm which checks if input["scales"] exists, and if not, use the defaults. I.e. something like

Let scales be the list « 1.0, 1.0, 1.0, 1.0 ».
If input["scales"] exists, then set scales to input["scales"].

(and maybe also validate that input["scales"] is of length 4, or whatever you need?)

This seems difficult since right now your spec has no algorithms (i.e. no method steps) for its operations, just a non-normative description of the inputs and return value.

We should make this a global update to all affected dictionaries. We probably want to define a reusable algorithm we reference from all these places.

Given this issue is specific to dictionaries with list defaults, for "method steps" convention in general, I opened #210

@inexorabletash
Copy link
Contributor

This is mostly taken care of by 0fe9e5c and prior changes, albeit with slightly different wording than suggested by @domenic

Suggested:

Let scales be the list « 1.0, 1.0, 1.0, 1.0 ».
If input["scales"] exists, then set scales to input["scales"].

Current:

If options.padding does not exist, set it to « 0, 0, 0, 0 ».

Differences:

  • Literal list syntax « 0, 0, 0, 0 » is used without "the list" prefix (linked to the Infra definition) - I missed this!
  • Dictionary member syntax - see Use web spec best practices #450 (comment) for some discussion
  • Unpacking to a local variable vs. mutating the dictionary. Either is fine; while the latter looks like it would modify the passed JS object but the WebIDL processing model means that doesn't happen. In most cases in the spec an options dict is passed verbatim to "... an implementation-defined platform operator for the ..., given options." (see Improve "underlying platform" references in spec #462 for some thoughts)

I'll put up a PR for the "the list" wording change, after that perhaps this can be closed?

inexorabletash added a commit to inexorabletash/webnn that referenced this issue Jan 17, 2024
Infra convention and recommended by webmachinelearning#211 - rather than the just «»
syntax, specify the Infra type of "list" and link to the definition.
@zolkis
Copy link
Collaborator

zolkis commented Jan 18, 2024

I'll put up a PR for the "the list" wording change, after that perhaps this can be closed?

I like the most compact formulation, which simplifies reading of a long algorithm:

If options.padding does not exist, set it to the list « 0, 0, 0, 0 ».

But I get the logic in Domenic's suggestion, especially when used in a shorter algorithm. However, when there are repetitions, IMHO the more compact version is far more readable.

inexorabletash added a commit to inexorabletash/webnn that referenced this issue Jan 18, 2024
Infra convention and recommended by webmachinelearning#211 - rather than the just «»
syntax, specify the Infra type of "list" and link to the definition.
inexorabletash added a commit to inexorabletash/webnn that referenced this issue Jan 21, 2024
Infra convention and recommended by webmachinelearning#211 - rather than the just «»
syntax, specify the Infra type of "list" and link to the definition.
@inexorabletash
Copy link
Contributor

layerNormalization() still has axes that have "assumed to be [1,2,3]" but nothing in the algorithm. I'll file a PR for that.

inexorabletash added a commit to inexorabletash/webnn that referenced this issue Jan 29, 2024
This adds an explicit algorithm step in layerNormalization() to
provide a default described only in prose for the axes option.

For webmachinelearning#211
inexorabletash added a commit to inexorabletash/webnn that referenced this issue Jan 29, 2024
This adds an explicit algorithm step in layerNormalization() to
provide a default described only in prose for the axes option.

For webmachinelearning#211
@inexorabletash
Copy link
Contributor

Testing: @webmachinelearning/triage folks should review and consider closing this after the above PR merges

inexorabletash added a commit to inexorabletash/webnn that referenced this issue Feb 6, 2024
This adds an explicit algorithm step in layerNormalization() to
provide a default described only in prose for the axes option.

For webmachinelearning#211
inexorabletash added a commit to inexorabletash/webnn that referenced this issue Feb 7, 2024
This adds an explicit algorithm step in layerNormalization() to
provide a default described only in prose for the axes option.

For webmachinelearning#211
@inexorabletash
Copy link
Contributor

I claim this can be closed now, so I'm gonna do it. New bugs if we missed any cleanup, please!

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

No branches or pull requests

4 participants