-
Notifications
You must be signed in to change notification settings - Fork 442
Tweak distribution of diskann-wide float tests.
#1305
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
base: main
Are you sure you want to change the base?
Changes from 3 commits
775aa1b
fc5ba8d
1963688
cb2026e
a84b6eb
f2ca48a
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -53,7 +53,7 @@ impl Layout for f32 { | |
| pub struct Finite; | ||
|
|
||
| macro_rules! finite { | ||
| ($T:ty, $bits:ty) => { | ||
| ($T:ty, $bits:ty, $twice:ty) => { | ||
| impl Distribution<$T> for Finite { | ||
| /// Generate floating point numbers spread more-or-less uniformly across the | ||
| /// distribution of floating point numbers. | ||
|
|
@@ -68,13 +68,23 @@ macro_rules! finite { | |
| /// | ||
| /// This function does not generate infinities or NaNs. | ||
| fn sample<R: Rng + ?Sized>(&self, rng: &mut R) -> $T { | ||
| // Generate a uniformly distributed 32-bit integer | ||
| let mut value: $bits = StandardUniform {}.sample(rng); | ||
|
|
||
| // The distribution from which we sample weights to determine the type of | ||
| // floating point number we are going to generate. | ||
| let weight = value % 100; | ||
| let (mask, allow_edge_exponent, allow_zero_mantissa) = if weight < 90 { | ||
| // Generate a uniformly distributed integer. | ||
| // | ||
| // This integer is twice as large as what's actually needed to generate | ||
| // | ||
| // * the value that will be used to make the final floating point number | ||
| // (the lower bits). | ||
| // | ||
| // * a selector for the kind of floating point number we are going to | ||
| // generate (the upper bits). | ||
| // | ||
| // Generating a number twice as big allows us to perform just a single sample | ||
| // from the random number generator without biasing the result. | ||
| let twice: $twice = StandardUniform {}.sample(rng); | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The aggregate frequency checks do not directly cover the regression this change fixes: selector/payload independence. An implementation that accidentally reused the lower bits for both could restore correlated or unreachable float bit patterns while preserving the tested category totals. Please add a deterministic RNG regression test for both f16 and f32 that controls the upper selector bits and lower payload bits independently, verifying that each half can vary without influencing the other.
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Thanks, but I'd rather not. Such a test would overfit the implementation and not test the distribution within each category, which really requires statistical methods. If this were a publicly exported distribution, such statistical tests would be warranted. However, the goal of this trait is to generate floating point numbers with sufficient distribution to catch high level numerical differences between implementations, for which even the previous biased implementation was sufficient. I don't believe the engineering ROI is there, particularly since the integers used to compose the final result are drawn from a uniform distribution already. |
||
|
|
||
| let mut value = twice as $bits; | ||
| let kind = (twice >> <$bits>::BITS) % 128; | ||
| let (mask, allow_edge_exponent, allow_zero_mantissa) = if kind < 116 { | ||
| // Generate a normal floating point number. | ||
| // | ||
| // All digits are fair game, but the exponent cannot be all zeros | ||
|
|
@@ -83,7 +93,7 @@ macro_rules! finite { | |
| // | ||
| // The mantissa is allowed to be all zeros. | ||
| (<$T>::EXPONENT_MASK | <$T>::MANTISSA_MASK, false, true) | ||
| } else if weight < 95 { | ||
| } else if kind < 122 { | ||
| // Generate a subnormal floating point number. | ||
| // | ||
| // The exponent must be all zero and the mantissa cannot be zero. | ||
|
|
@@ -117,8 +127,8 @@ macro_rules! finite { | |
| }; | ||
| } | ||
|
|
||
| finite!(half::f16, u16); | ||
| finite!(f32, u32); | ||
| finite!(half::f16, u16, u32); | ||
| finite!(f32, u32, u64); | ||
|
|
||
| /////////// | ||
| // Tests // | ||
|
|
@@ -231,9 +241,9 @@ mod tests { | |
| where | ||
| T: TestDistribution, | ||
| { | ||
| let normal_weight = 90; | ||
| let subnormal_weight = 5; | ||
| let zero_weight = 5; | ||
| let normal_weight = 116; | ||
|
hildebrandmw marked this conversation as resolved.
Outdated
|
||
| let subnormal_weight = 6; | ||
| let zero_weight = 6; | ||
| let total_weight = normal_weight + subnormal_weight + zero_weight; | ||
|
|
||
| let num_trials: i64 = 1_000_000; | ||
|
|
||
Uh oh!
There was an error while loading. Please reload this page.