pitrou commented on code in PR #48224:
URL: https://github.com/apache/arrow/pull/48224#discussion_r2565544053
##########
cpp/src/arrow/testing/math.cc:
##########
@@ -17,53 +17,107 @@
#include "arrow/testing/math.h"
+#include <algorithm>
#include <cmath>
#include <limits>
+#include <type_traits>
#include <gtest/gtest.h>
+#include "arrow/util/float16.h"
#include "arrow/util/logging_internal.h"
namespace arrow {
namespace {
template <typename Float>
-bool WithinUlpOneWay(Float left, Float right, int n_ulps) {
- // The delta between 1.0 and the FP value immediately before it.
- // We're using this value because `frexp` returns a mantissa between 0.5 and
1.0.
- static const Float kOneUlp = Float(1.0) - std::nextafter(Float(1.0),
Float(0.0));
+struct FloatToUInt;
- DCHECK_GE(n_ulps, 1);
+template <>
+struct FloatToUInt<double> {
+ using Type = uint64_t;
+};
- if (left == 0) {
- return left == right;
+template <>
+struct FloatToUInt<float> {
+ using Type = uint32_t;
+};
+
+template <>
+struct FloatToUInt<util::Float16> {
+ using Type = uint16_t;
+};
+
+template <typename Float>
+struct UlpDistanceUtil {
+ public:
+ using UIntType = typename FloatToUInt<Float>::Type;
+ static const UIntType kNumberOfBits = sizeof(Float) * 8;
+ static const UIntType kSignMask = static_cast<UIntType>(1) << (kNumberOfBits
- 1);
+
+ // This implementation is inspired by:
+ //
https://randomascii.wordpress.com/2012/02/25/comparing-floating-point-numbers-2012-edition/
+ static UIntType UlpDistnace(Float left, Float right) {
+ auto unsigned_left = BitCast<UIntType>(left);
+ auto unsigned_right = BitCast<UIntType>(right);
+ auto biased_left = ConvertSignAndMagnitudeToBiased(unsigned_left);
+ auto biased_right = ConvertSignAndMagnitudeToBiased(unsigned_right);
+ if (biased_left > biased_right) {
+ std::swap(biased_left, biased_right);
+ }
+ return biased_right - biased_left;
}
- if (left < 0) {
- left = -left;
- right = -right;
+
+ private:
+ template <typename To, typename From>
+ union BitCastUnion {
+ explicit BitCastUnion(From from) : from(from) {}
+ From from;
+ To to;
+ };
+
+ template <typename To, typename From>
+ static UIntType BitCast(From value) {
+ assert(sizeof(To) == sizeof(From));
+ BitCastUnion<To, From> bit_cast(value);
+ return bit_cast.to;
}
- int left_exp;
- Float left_mant = std::frexp(left, &left_exp);
- Float delta = static_cast<Float>(n_ulps) * kOneUlp;
- Float lower_bound = std::ldexp(left_mant - delta, left_exp);
- Float upper_bound = std::ldexp(left_mant + delta, left_exp);
- return right >= lower_bound && right <= upper_bound;
-}
+ // Source reference (GoogleTest):
+ //
https://github.com/google/googletest/blob/085af2cc08600bdb13827ca40261abcbe5048bb5/googletest/include/gtest/internal/gtest-internal.h#L336-L342
Review Comment:
Please let's reference the actual function:
https://github.com/google/googletest/blob/1b96fa13f549387b7549cc89e1a785cf143a1a50/googletest/include/gtest/internal/gtest-internal.h#L345-L368
##########
cpp/src/arrow/testing/math.h:
##########
@@ -21,11 +21,19 @@
namespace arrow {
+namespace util {
+class Float16;
+}
Review Comment:
You can include `"arrow/type_fwd.h"` instead.
##########
cpp/src/arrow/testing/math.cc:
##########
@@ -17,53 +17,107 @@
#include "arrow/testing/math.h"
+#include <algorithm>
#include <cmath>
#include <limits>
+#include <type_traits>
#include <gtest/gtest.h>
+#include "arrow/util/float16.h"
#include "arrow/util/logging_internal.h"
namespace arrow {
namespace {
template <typename Float>
-bool WithinUlpOneWay(Float left, Float right, int n_ulps) {
- // The delta between 1.0 and the FP value immediately before it.
- // We're using this value because `frexp` returns a mantissa between 0.5 and
1.0.
- static const Float kOneUlp = Float(1.0) - std::nextafter(Float(1.0),
Float(0.0));
+struct FloatToUInt;
- DCHECK_GE(n_ulps, 1);
+template <>
+struct FloatToUInt<double> {
+ using Type = uint64_t;
+};
- if (left == 0) {
- return left == right;
+template <>
+struct FloatToUInt<float> {
+ using Type = uint32_t;
+};
+
+template <>
+struct FloatToUInt<util::Float16> {
+ using Type = uint16_t;
+};
+
+template <typename Float>
+struct UlpDistanceUtil {
+ public:
+ using UIntType = typename FloatToUInt<Float>::Type;
+ static const UIntType kNumberOfBits = sizeof(Float) * 8;
+ static const UIntType kSignMask = static_cast<UIntType>(1) << (kNumberOfBits
- 1);
+
+ // This implementation is inspired by:
+ //
https://randomascii.wordpress.com/2012/02/25/comparing-floating-point-numbers-2012-edition/
+ static UIntType UlpDistnace(Float left, Float right) {
+ auto unsigned_left = BitCast<UIntType>(left);
+ auto unsigned_right = BitCast<UIntType>(right);
+ auto biased_left = ConvertSignAndMagnitudeToBiased(unsigned_left);
+ auto biased_right = ConvertSignAndMagnitudeToBiased(unsigned_right);
+ if (biased_left > biased_right) {
+ std::swap(biased_left, biased_right);
+ }
+ return biased_right - biased_left;
}
- if (left < 0) {
- left = -left;
- right = -right;
+
+ private:
+ template <typename To, typename From>
+ union BitCastUnion {
+ explicit BitCastUnion(From from) : from(from) {}
+ From from;
+ To to;
+ };
+
+ template <typename To, typename From>
+ static UIntType BitCast(From value) {
+ assert(sizeof(To) == sizeof(From));
+ BitCastUnion<To, From> bit_cast(value);
+ return bit_cast.to;
}
Review Comment:
You don't need this, you can just use `SafeCopy<UIntType>` (see
`arrow/util/ubsan.h`)
##########
cpp/src/arrow/testing/math.cc:
##########
@@ -17,53 +17,107 @@
#include "arrow/testing/math.h"
+#include <algorithm>
#include <cmath>
#include <limits>
+#include <type_traits>
#include <gtest/gtest.h>
+#include "arrow/util/float16.h"
#include "arrow/util/logging_internal.h"
namespace arrow {
namespace {
template <typename Float>
-bool WithinUlpOneWay(Float left, Float right, int n_ulps) {
- // The delta between 1.0 and the FP value immediately before it.
- // We're using this value because `frexp` returns a mantissa between 0.5 and
1.0.
- static const Float kOneUlp = Float(1.0) - std::nextafter(Float(1.0),
Float(0.0));
+struct FloatToUInt;
- DCHECK_GE(n_ulps, 1);
+template <>
+struct FloatToUInt<double> {
+ using Type = uint64_t;
+};
- if (left == 0) {
- return left == right;
+template <>
+struct FloatToUInt<float> {
+ using Type = uint32_t;
+};
+
+template <>
+struct FloatToUInt<util::Float16> {
+ using Type = uint16_t;
+};
+
+template <typename Float>
+struct UlpDistanceUtil {
+ public:
+ using UIntType = typename FloatToUInt<Float>::Type;
+ static const UIntType kNumberOfBits = sizeof(Float) * 8;
+ static const UIntType kSignMask = static_cast<UIntType>(1) << (kNumberOfBits
- 1);
Review Comment:
Please let's make these `constexpr`.
##########
cpp/src/arrow/testing/math.cc:
##########
@@ -17,53 +17,107 @@
#include "arrow/testing/math.h"
+#include <algorithm>
#include <cmath>
#include <limits>
+#include <type_traits>
#include <gtest/gtest.h>
+#include "arrow/util/float16.h"
#include "arrow/util/logging_internal.h"
namespace arrow {
namespace {
template <typename Float>
-bool WithinUlpOneWay(Float left, Float right, int n_ulps) {
- // The delta between 1.0 and the FP value immediately before it.
- // We're using this value because `frexp` returns a mantissa between 0.5 and
1.0.
- static const Float kOneUlp = Float(1.0) - std::nextafter(Float(1.0),
Float(0.0));
+struct FloatToUInt;
- DCHECK_GE(n_ulps, 1);
+template <>
+struct FloatToUInt<double> {
+ using Type = uint64_t;
+};
- if (left == 0) {
- return left == right;
+template <>
+struct FloatToUInt<float> {
+ using Type = uint32_t;
+};
+
+template <>
+struct FloatToUInt<util::Float16> {
+ using Type = uint16_t;
+};
+
+template <typename Float>
+struct UlpDistanceUtil {
+ public:
+ using UIntType = typename FloatToUInt<Float>::Type;
+ static const UIntType kNumberOfBits = sizeof(Float) * 8;
+ static const UIntType kSignMask = static_cast<UIntType>(1) << (kNumberOfBits
- 1);
+
+ // This implementation is inspired by:
+ //
https://randomascii.wordpress.com/2012/02/25/comparing-floating-point-numbers-2012-edition/
+ static UIntType UlpDistnace(Float left, Float right) {
Review Comment:
Please fix typo: "UlpDistnace"
--
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]
