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

Content change: Add definitions for MLOperand's shape and dataType #555

Merged

Conversation

inexorabletash
Copy link
Member

@inexorabletash inexorabletash commented Feb 5, 2024

Simplify text that digs the dimensions and dataType out of an MLOperand's [[descriptor]] by introducing definitions, and use them consistently.

Also simplify the definition for MLOperand's rank, and use it in more places.


Preview | Diff

@inexorabletash
Copy link
Member Author

This will conflict with some other PRs in flight so for now just review as "is this an improvement?"

@zolkis
Copy link
Collaborator

zolkis commented Feb 5, 2024

Looks good.

@inexorabletash inexorabletash force-pushed the content-operand-properties branch from 5da50b2 to f8e91a2 Compare February 6, 2024 18:11
@inexorabletash inexorabletash marked this pull request as draft February 6, 2024 18:15
Simplify text that digs the dimensions and dataType out of an
MLOperand's [[descriptor]] by introducing definitions, and use them
consistently.

Also simplify the definition for MLOperand's rank, and use it
in more places.
@inexorabletash inexorabletash force-pushed the content-operand-properties branch from f8e91a2 to 5d296a7 Compare February 7, 2024 17:43
@inexorabletash inexorabletash mentioned this pull request Feb 7, 2024
Copy link
Collaborator

@fdwr fdwr left a comment

Choose a reason for hiding this comment

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

"is this an improvement?" - I think so. Thanks.

@fdwr
Copy link
Collaborator

fdwr commented Feb 13, 2024

@inexorabletash : Please promote from draft when ready. 0D is merged.

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!

@inexorabletash inexorabletash marked this pull request as ready for review February 13, 2024 19:09
@inexorabletash
Copy link
Member Author

Okay, merged the changes from main. I didn't force-push so the history has a merge commit. Hopefully I did it right!

Should be ready for review.

Copy link
Collaborator

@fdwr fdwr left a comment

Choose a reason for hiding this comment

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

SLGTM. TY.

@fdwr fdwr merged commit 8503e24 into webmachinelearning:main Feb 13, 2024
2 checks passed
github-actions bot added a commit that referenced this pull request Feb 13, 2024
)

SHA: 8503e24
Reason: push, by fdwr

Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
@inexorabletash inexorabletash deleted the content-operand-properties branch February 13, 2024 23:05
inexorabletash added a commit to inexorabletash/webnn that referenced this pull request Feb 14, 2024
Old a busted: ... the [=property=] of |object| ...

New hotness: ... |object|'s [=property=] ...

Discussed in webmachinelearning#555 (comment)
inexorabletash added a commit to inexorabletash/webnn that referenced this pull request Feb 14, 2024
Old and busted: ... the [=property=] of |object| ...

New hotness: ... |object|'s [=property=] ...

Discussed in webmachinelearning#555 (comment)
anssiko pushed a commit that referenced this pull request Feb 15, 2024
…565)

* Bug fix: Correct "rank of op's shape" to just "rank of op"

MLOperands have a rank; the shape just has a size.

* Wording: Use shorter possessive form for abstract object properties

Old and busted: ... the [=property=] of |object| ...
New hotness: ... |object|'s [=property=] ...
Discussed in #555 (comment)

* Bug fix: Dedupe some words

I noticed "then then" in the previous commit and searched for similar
glitches to fix.

* Bug fix: Incorrect referencing operand properties

* Erroneously referencing descriptor properties, but directly against
  an operand. This can be fixed by using the shape and dataType
  shortcuts. (6 cases)

* Simplifications where an operand's shape shortcut could be used. (3
  cases)
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