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

algorithms: Add bhalf rand support #4653

Merged
merged 1 commit into from
Jan 27, 2022

Conversation

e10harvey
Copy link
Contributor

Supersedes #4652.
Related to #4372.

@e10harvey e10harvey added the Feature Request Create new capability; will potentially require voting label Jan 7, 2022
@e10harvey e10harvey self-assigned this Jan 7, 2022
@e10harvey e10harvey added this to In progress in Developer: E10HARVEY via automation Jan 7, 2022
@e10harvey
Copy link
Contributor Author

retest this please

@@ -424,6 +432,12 @@ struct test_random_scalar {
}
#endif

#if defined(KOKKOS_BHALF_T_IS_FLOAT) && !KOKKOS_BHALF_T_IS_FLOAT
if (std::is_same<Scalar, Kokkos::Experimental::bhalf_t>::value) {
variance_factor = 15.01;
Copy link
Member

Choose a reason for hiding this comment

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

Where does this number come from?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Based on the test failure output with a smaller variance factor, this is approximately the smallest variance factor I found that allows the tests to pass.

Comment on lines +391 to +393
mean_eps_expect = 0.019;
variance_eps_expect = 1.0;
covariance_eps_expect = 2.8e4;
Copy link
Member

Choose a reason for hiding this comment

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

How did you determine these numbers?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Same as above.

@e10harvey
Copy link
Contributor Author

@crtrott Is there a way to calculate the expected variance factor and epsilons based on the scalar precision?

@e10harvey
Copy link
Contributor Author

retest this please

@e10harvey e10harvey moved this from In progress to Review in progress in Developer: E10HARVEY Jan 17, 2022
@e10harvey e10harvey requested a review from dalg24 January 17, 2022 19:58
@crtrott crtrott added the Blocks Promotion Overview issue for release-blocking bugs label Jan 19, 2022
@nmm0 nmm0 added this to In progress in Kokkos Release 3.6 via automation Jan 19, 2022
Copy link
Member

@dalg24 dalg24 left a comment

Choose a reason for hiding this comment

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

Looks good besides the magic numbers
Waiting to hear back on #4653 (comment)

@ajpowelsnl
Copy link
Contributor

Retest this please

@crtrott
Copy link
Member

crtrott commented Jan 25, 2022

There is certainly a way to calculate where these numbers come from, but its likely pretty complicated because the semi randomness of roundof errors plays a role here, which for half and bhalf are not enough orders of magnitude away from the actual random generator randomness. Unfortunately I doubt its a fully uncorrelated distribution which makes this all iffy. I don't have time to work the math out at least.

@e10harvey
Copy link
Contributor Author

There is certainly a way to calculate where these numbers come from, but its likely pretty complicated because the semi randomness of roundof errors plays a role here, which for half and bhalf are not enough orders of magnitude away from the actual random generator randomness. Unfortunately I doubt its a fully uncorrelated distribution which makes this all iffy. I don't have time to work the math out at least.

Would you like the unit-tests removed? We need this PR for kokkos/kokkos-kernels#1251.

@dalg24
Copy link
Member

dalg24 commented Jan 26, 2022

Retest this please

Developer: E10HARVEY automation moved this from Review in progress to Reviewer approved Jan 26, 2022
@rgayatri23
Copy link
Contributor

The OpenMPTarget failure is unrelated to this issue. Seems like the OpenMPTarget CI is having issues.

@e10harvey
Copy link
Contributor Author

@crtrott: Ready to merge?

@crtrott crtrott merged commit c3ec4fa into kokkos:develop Jan 27, 2022
Kokkos Release 3.6 automation moved this from In progress to Done Jan 27, 2022
Developer: E10HARVEY automation moved this from Reviewer approved to Done Jan 27, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Blocks Promotion Overview issue for release-blocking bugs Feature Request Create new capability; will potentially require voting
Development

Successfully merging this pull request may close these issues.

None yet

5 participants