Jefffrey commented on code in PR #11344:
URL: https://github.com/apache/arrow-rs/pull/11344#discussion_r4383940584
##########
arrow-buffer/src/bigint/mod.rs:
##########
@@ -1977,6 +1987,93 @@ mod tests {
}
}
+ #[test]
+ fn test_i256_to_f32_positive_midpoint() {
+ let value = (1_i128 << 53) + (1_i128 << 29) + 1;
Review Comment:
can we have an explanation to these numbers? why are we shifting by 53 and
29 and adding them here?
##########
arrow-buffer/src/bigint/mod.rs:
##########
@@ -1977,6 +1987,93 @@ mod tests {
}
}
+ #[test]
+ fn test_i256_to_f32_positive_midpoint() {
+ let value = (1_i128 << 53) + (1_i128 << 29) + 1;
+ assert_eq!(
+ i256::from_i128(value).to_f32().unwrap().to_bits(),
+ (value as f32).to_bits()
+ );
+ }
+
+ #[test]
+ fn test_i256_to_f32_negative_midpoint() {
+ let value = -((1_i128 << 53) + (1_i128 << 29) + 1);
+ assert_eq!(
+ i256::from_i128(value).to_f32().unwrap().to_bits(),
+ (value as f32).to_bits()
+ );
+ }
+
+ #[test]
+ fn test_i256_to_f32_midpoints() {
+ for exponent in 24_u32..=127 {
+ let step = i256::ONE << (exponent - 23);
+ for fraction in [0, 1, 0x7fffff] {
+ let lower = (i256::ONE << exponent) + step *
i256::from_i128(i128::from(fraction));
+ let midpoint = lower + (step >> 1_u32);
+ let lower_bits = ((exponent + 127) << 23) | fraction;
+ for offset in [-1, 0, 1] {
+ let value: i256 = midpoint + i256::from(offset);
+ let round_up = offset > 0 || (offset == 0 && fraction & 1
== 1);
+ let expected = lower_bits + u32::from(round_up);
+ assert_eq!(value.to_f32().unwrap().to_bits(), expected,
"{value}");
+ assert_eq!(
+ (-value).to_f32().unwrap().to_bits(),
+ expected | 0x80000000,
+ "{}",
+ -value
+ );
+ }
+ }
+ }
+ }
+
+ #[test]
+ fn test_i256_to_f32_extrema() {
+ let largest_finite = (i256::ONE << 128_u32) - (i256::ONE << 104_u32);
+ for (value, expected) in [
+ (i256::ZERO, 0.0),
+ (i256::ONE, 1.0),
+ (i256::MINUS_ONE, -1.0),
+ (largest_finite, f32::MAX),
+ (-largest_finite, -f32::MAX),
+ (i256::ONE << 128_u32, f32::INFINITY),
+ (-(i256::ONE << 128_u32), f32::NEG_INFINITY),
+ (i256::MIN, f32::NEG_INFINITY),
+ (i256::MAX, f32::INFINITY),
+ ] {
+ assert_eq!(
+ value.to_f32().unwrap().to_bits(),
+ expected.to_bits(),
+ "{value}"
+ );
+ }
+ for value in [i128::MIN, i128::MAX] {
+ assert_eq!(
+ i256::from_i128(value).to_f32().unwrap().to_bits(),
+ (value as f32).to_bits()
+ );
+ }
+ }
+
+ #[test]
+ fn test_i256_to_f32_full_range() {
+ let mut state = 42_u128;
+ for shift in 0..256_u32 {
+ state = state.wrapping_mul(6364136223846793005).wrapping_add(1);
Review Comment:
is this an rng generator? why are we handrolling this?
##########
arrow-buffer/src/bigint/mod.rs:
##########
@@ -1977,6 +1987,93 @@ mod tests {
}
}
+ #[test]
+ fn test_i256_to_f32_positive_midpoint() {
+ let value = (1_i128 << 53) + (1_i128 << 29) + 1;
+ assert_eq!(
+ i256::from_i128(value).to_f32().unwrap().to_bits(),
+ (value as f32).to_bits()
+ );
+ }
+
+ #[test]
+ fn test_i256_to_f32_negative_midpoint() {
+ let value = -((1_i128 << 53) + (1_i128 << 29) + 1);
Review Comment:
this test can be combined with the above to simplify the boilerplate
##########
arrow-buffer/src/bigint/mod.rs:
##########
@@ -1977,6 +1987,93 @@ mod tests {
}
}
+ #[test]
+ fn test_i256_to_f32_positive_midpoint() {
+ let value = (1_i128 << 53) + (1_i128 << 29) + 1;
+ assert_eq!(
+ i256::from_i128(value).to_f32().unwrap().to_bits(),
+ (value as f32).to_bits()
+ );
+ }
+
+ #[test]
+ fn test_i256_to_f32_negative_midpoint() {
+ let value = -((1_i128 << 53) + (1_i128 << 29) + 1);
+ assert_eq!(
+ i256::from_i128(value).to_f32().unwrap().to_bits(),
+ (value as f32).to_bits()
+ );
+ }
+
+ #[test]
+ fn test_i256_to_f32_midpoints() {
Review Comment:
this test also needs better explanation; theres too many magic numbers here
that rely on some previous knowledge (i assume related to the range of values
representable by a f32?)
not to mention how many parameters it just dumps out
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
