Skip to content

Comments

Introduce "valid dimension", used as needed when calculating operand shapes#641

Merged
fdwr merged 5 commits intowebmachinelearning:mainfrom
inexorabletash:bugfix-resample2d-numeric-limit
Apr 16, 2024
Merged

Introduce "valid dimension", used as needed when calculating operand shapes#641
fdwr merged 5 commits intowebmachinelearning:mainfrom
inexorabletash:bugfix-resample2d-numeric-limit

Conversation

@inexorabletash
Copy link
Contributor

@inexorabletash inexorabletash commented Apr 8, 2024 •
edited by pr-preview bot
Loading

Certain operands like reshape2d() calculate dimensions in their output shape with expressions like shape[i] * scales[i] which could be larger than can be represented as a dimension in MLOperandDescriptor.

Introduce a "valid dimension" term, use it when calculating output dimensions in the following ops, and throw if needed.

  • constant() (sequential fill)
  • concat()
  • conv2d()
  • convTranspose2d()
  • pad()
  • pooling operators
  • resample2d()
  • reshape()

Fixes #610

Preview | Diff

@inexorabletash
resample2d: Validate that dimensions are valid unsigned longs
6563518 
If explicit sizes aren't provided, the size of an output dimension is
calculated as shape[i] * scales[i] which could be larger than can be
represented as a dimension in MLOperandDescriptor.

Explicitly validate and throw if this constraint isn't satisfied.

Fixes #610
@inexorabletash
Copy link
Contributor Author

inexorabletash commented Apr 8, 2024

Up for discussion. A few more thoughts here:

A valid dimension is an integer greater than zero in the range of unsigned long. Implementations may impose a lower upper limit.

(dropping "greater than zero" depending on #391)

... and then change check dimensions to say:

If any element of descriptor.dimensions is not a valid dimension, then return false.

Thoughts?

@inexorabletash
Copy link
Contributor Author

inexorabletash commented Apr 8, 2024

If we introduce a "valid dimension" definition, we probably want to use it in these other places that calculate a dimension:

... which are the obvious places where a dimension is assigned from a calculation. Probably more places.

@huningxin
Copy link
Contributor

huningxin commented Apr 9, 2024

Probably more places.

  • concat
  • gru and gruCell, their steps compare a dimension with 3 * hiddenSize that might be a invalid unsigned long.
  • lstm and lstmCell, their steps compare a dimension with 4 * hiddenSize that might be a invalid unsigned long.

@inexorabletash inexorabletash changed the title resample2d: Validate that dimensions are valid unsigned longs resample2d(): Validate that dimensions are valid unsigned longs Apr 9, 2024
@inexorabletash
Copy link
Contributor Author

inexorabletash commented Apr 9, 2024 •
edited
Loading

Ah yes, it's a sum of other dimensions, so could overflow.

  • gru and gruCell, their steps compare a dimension with 3 * hiddenSize that might be a invalid unsigned long.
  • lstm and lstmCell, their steps compare a dimension with 4 * hiddenSize that might be a invalid unsigned long.

I think those are cases where the intermediate value could overflow if the math is done in uint32 space, but the final values used for dimensions are always valid uint32, since they come directly from other MLOperands. So those are cases where #636 applies.

I'm gonna update this PR to introduce the "valid dimension" term (done in e737dcb). Should I go ahead and expand it beyond resample2d() or do that in another PR?

huningxin
huningxin approved these changes Apr 10, 2024
View reviewed changes
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!

@huningxin
Copy link
Contributor

huningxin commented Apr 10, 2024

Thanks @inexorabletash !

Should I go ahead and expand it beyond resample2d() or do that in another PR?

Given "valid dimension" is defined, having all its usages in this PR sounds good to me. But I am also fine if doing it in another PR.

@inexorabletash
Copy link
Contributor Author

inexorabletash commented Apr 10, 2024

Given "valid dimension" is defined, having all its usages in this PR sounds good to me. But I am also fine if doing it in another PR.

Done in 4f30583 - I'll update the PR name / description

@inexorabletash inexorabletash changed the title resample2d(): Validate that dimensions are valid unsigned longs Introduce "valid dimension", used as needed when calculating operand shapes Apr 10, 2024
huningxin
huningxin approved these changes Apr 11, 2024
View reviewed changes
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 with a nit

@inexorabletash @huningxin
Update index.bs
e747fbb 
Co-authored-by: Ningxin Hu <[email protected]>
@inexorabletash inexorabletash requested a review from fdwr April 15, 2024 16:20
@inexorabletash
Copy link
Contributor Author

inexorabletash commented Apr 15, 2024

@fdwr - can you please review and merge if it looks good to you? Thanks!

fdwr
fdwr approved these changes Apr 16, 2024
View reviewed changes
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.

Tiny wording thought (do you prefer one over the other? I'll go with whatever you like.), else LGTM. Thanks, as I like centralizing this dimension validity rather than it being scattered around.

@inexorabletash @fdwr
Update index.bs
67027f2 
Co-authored-by: Dwayne Robinson <[email protected]>
@inexorabletash
Copy link
Contributor Author

inexorabletash commented Apr 16, 2024

Suggested wording change was great @fdwr - it felt awkward as I was writing it.

@fdwr fdwr merged commit db0ed6a into webmachinelearning:main Apr 16, 2024
github-actions bot added a commit that referenced this pull request Apr 16, 2024
@inexorabletash @github-actions
Introduce "valid dimension", used as needed when calculating operand …
2ecd439 
…shapes (#641)

SHA: db0ed6a
Reason: push, by fdwr

Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
@inexorabletash inexorabletash deleted the bugfix-resample2d-numeric-limit branch April 16, 2024 15:54
@dontcallmedom dontcallmedom mentioned this pull request Jan 15, 2026
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@huningxin huningxin huningxin approved these changes

@fdwr fdwr fdwr approved these changes

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Need clarify scale factor for resample2d

3 participants

@inexorabletash @huningxin @fdwr