andishgar commented on code in PR #48224:
URL: https://github.com/apache/arrow/pull/48224#discussion_r2588363166
##########
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:
@pitrou
Thanks for the answer.
> Also, the usage of `union` in this PR is undefined behavior, which is why
we must use another idiom. See
https://en.cppreference.com/w/cpp/language/union.html:
>
> > It is undefined behavior to read from the member of the union that
wasn't most recently written.
Thank you for the clarification. I understand now why this approach is not
appropriate.
> > 2-This only works for types that have a default constructor. However, it
is possible for a type to be trivially copyable without having a default
constructor. Shouldn’t this be considered a bug?
>
> I don't know, is this relevant to using it with floating-point numbers?
It's not relevant to this issue. I will open a separate issue for it.
Just for clarification, the following type is trivially copyable but does
not have a default constructor:
```c++
struct A{
A(int a):a(a){}
int a;
}
```
--
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]
