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: Add types to all internal algorithm members #536

Merged
merged 1 commit into from
Feb 6, 2024

Conversation

inexorabletash
Copy link
Member

@inexorabletash inexorabletash commented Jan 27, 2024

Note that this isn't TypeScript or C++ - we are free to make the type annotations more specific, easier to read, etc. So there may be cases where we don't simply give an Infra or Web IDL type or rigorously follow the Infra style. Comments and suggestions welcome!


Preview | Diff

@inexorabletash
Copy link
Member Author

One potential point of discussion - instead of just "string" e.g. in "To create argMin/argMax operation given string op, ..." we could more specifically say "operator name". And we could go further and give a definition for that term ("A operator name is a string, used internally to distinguish similar operators..."), but that's overkill IMHO.

@huningxin
Copy link
Contributor

And we could go further and give a definition for that term ("A operator name is a string, used internally to distinguish similar operators...")

Or we can define an enum having all valid operator names? Maybe also overkill.

Copy link
Contributor

@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, thanks much!

Copy link
Member

@anssiko anssiko left a comment

Choose a reason for hiding this comment

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

👍

@inexorabletash
Copy link
Member Author

And just to be pedantic: I now have Triage permissions (thanks!) but not Write permissions, so I still need an editor (etc) to hit the Merge button even if a PR has multiple approvals. Thanks in advance @wchao1115 😄

@anssiko
Copy link
Member

anssiko commented Feb 6, 2024

I will merge this last open "wording change" ready-to-merge PR to let @wchao1115 focus his attention on PRs that propose normative changes.

I will ask @inexorabletash to give an overview of the recent wording change PRs on our upcoming meeting to ensure the entire WG is informed of changes that go in.

@anssiko anssiko merged commit c9d2dd7 into webmachinelearning:main Feb 6, 2024
2 checks passed
github-actions bot added a commit that referenced this pull request Feb 6, 2024
SHA: c9d2dd7
Reason: push, by anssiko

Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
@inexorabletash inexorabletash deleted the wording-algo-types branch February 6, 2024 15:38
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.

3 participants