-
Notifications
You must be signed in to change notification settings - Fork 743
Support Half/Bfloat for rand() and fill(). #8123
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
Conversation
Summary: . Differential Revision: D68984778
🔗 Helpful Links🧪 See artifacts and rendered test results at hud.pytorch.org/pr/pytorch/executorch/8123
Note: Links to docs will display an error until the docs builds have been completed. ⏳ No Failures, 2 PendingAs of commit b132696 with merge base dd8da0f ( This comment was automatically generated by Dr. CI and updates every 15 minutes. |
|
This pull request was exported from Phabricator. Differential Revision: D68984778 |
|
@pytorchbot label "topic: not user facing" |
| for (auto i = 0; i < tensor->numel(); ++i) { | ||
| auto val = tensor->const_data_ptr<executorch::aten::BFloat16>()[i]; | ||
| EXPECT_GE(val, 0.0); | ||
| EXPECT_LT(val, 1.0); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm confused. doesn't rand generate normally distributed numbers? the mean is set to 0 and stddev is 1, and the normal distribution can certainly include numbers 1 or more standard deviations away from the mean.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I guess what you described is PyTorch's randn()?
Pretty confusing naming though, I agree.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I forgot to mention that I brought this up because this test is flaking; val is sometimes 1.0
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I guess what you described is PyTorch's randn()?
you're right: I misread earlier. however, the test is still flaking. see inline code comment
| std::default_random_engine gen{std::random_device{}()}; | ||
|
|
||
| ET_SWITCH_REALB_TYPES(type, nullptr, "random_strided", CTYPE, [&] { | ||
| ET_SWITCH_REALHBBF16_TYPES(type, nullptr, "random_strided", CTYPE, [&] { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think the problem is here -- there are presumably fp32 values that, while not 1.0f, convert to bfloat16 as 1.0 thanks to round-to-nearest-even.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So, I think both rand_strided and randint_strided (the two functions that use a uniform distribution on a half-open interval) need special handling not to generate the excluded endpoint of their interval. PyTorch's implementation seems to suggest this is a bit of a pain: https://github.com/pytorch/pytorch/blob/dcac3c3e06556bc0e729dd1fa75f4f1e81caa356/aten/src/ATen/native/DistributionTemplates.h#L30
Technically, code sharing support landed today, so we could refactor update_to and update_from out of this header and share them...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hope #8340 should be enough to fix the flakiness.
Summary: .
Differential Revision: D68984778