You are viewing a plain text version of this content. The canonical link for it is here.
Posted to github@arrow.apache.org by "andishgar (via GitHub)" <gi...@apache.org> on 2025/11/22 10:21:42 UTC
[PR] GH-48123 [C++][Float16] Reimplement arrow::WithinUlp and Enable it for float16 [arrow]
andishgar opened a new pull request, #48224:
URL: https://github.com/apache/arrow/pull/48224
### Rationale for this change
Refer to [this comment](https://github.com/apache/arrow/issues/48123#issuecomment-3561813079). Additionally, this change enables `arrow::WithinUlp` for `float16`.
### What changes are included in this PR?
Re-implement `arrow::WithinUlp` and enable it for `float16`, including relevant tests for corner cases around powers of two and `Float16`.
### Are these changes tested?
Yes, I ran the relevant unit tests.
### Are there any user-facing changes?
No.
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-48123 [C++][Float16] Reimplement arrow::WithinUlp and Enable it for float16 [arrow]
Posted by "conbench-apache-arrow[bot] (via GitHub)" <gi...@apache.org>.
conbench-apache-arrow[bot] commented on PR #48224:
URL: https://github.com/apache/arrow/pull/48224#issuecomment-3677933375
After merging your PR, Conbench analyzed the 3 benchmarking runs that have been run so far on merge-commit f40d62e4ef3c26f4d1ee0c681ab0c1ca637cd441.
There were no benchmark performance regressions. 🎉
The [full Conbench report](https://github.com/apache/arrow/runs/58613762358) has more details. It also includes information about 16 possible false positives for unstable benchmarks that are known to sometimes produce them.
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-48123 [C++][Float16] Reimplement arrow::WithinUlp and Enable it for float16 [arrow]
Posted by "andishgar (via GitHub)" <gi...@apache.org>.
andishgar commented on code in PR #48224:
URL: https://github.com/apache/arrow/pull/48224#discussion_r2587651401
##########
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
Would you consider `SafeCopy<T, U>` appropriate for unaligned loads of trivially copyable types in this context?
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-48123 [C++][Float16] Reimplement arrow::WithinUlp and Enable it for float16 [arrow]
Posted by "pitrou (via GitHub)" <gi...@apache.org>.
pitrou merged PR #48224:
URL: https://github.com/apache/arrow/pull/48224
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-48123 [C++][Float16] Reimplement arrow::WithinUlp and Enable it for float16 [arrow]
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on PR #48224:
URL: https://github.com/apache/arrow/pull/48224#issuecomment-3626656570
Revision: 05977361c7dc51f01779df574f5bd8e97708b025
Submitted crossbow builds: [ursacomputing/crossbow @ actions-aeed0cd233](https://github.com/ursacomputing/crossbow/branches/all?query=actions-aeed0cd233)
|Task|Status|
|----|------|
|example-cpp-minimal-build-static|[](https://github.com/ursacomputing/crossbow/actions/runs/20027925806/job/57429612998)|
|example-cpp-minimal-build-static-system-dependency|[](https://github.com/ursacomputing/crossbow/actions/runs/20027926877/job/57429616496)|
|example-cpp-tutorial|[](https://github.com/ursacomputing/crossbow/actions/runs/20027926775/job/57429616098)|
|test-build-cpp-fuzz|[](https://github.com/ursacomputing/crossbow/actions/runs/20027926175/job/57429614301)|
|test-conda-cpp|[](https://github.com/ursacomputing/crossbow/actions/runs/20027925801/job/57429612958)|
|test-conda-cpp-valgrind|[](https://github.com/ursacomputing/crossbow/actions/runs/20027926467/job/57429615280)|
|test-cuda-cpp-ubuntu-22.04-cuda-11.7.1|[](https://github.com/ursacomputing/crossbow/actions/runs/20027925955/job/57429613681)|
|test-cuda-cpp-ubuntu-24.04-cuda-13.0.2|[](https://github.com/ursacomputing/crossbow/actions/runs/20027926372/job/57429614846)|
|test-debian-12-cpp-amd64|[](https://github.com/ursacomputing/crossbow/actions/runs/20027926488/job/57429615089)|
|test-debian-12-cpp-i386|[](https://github.com/ursacomputing/crossbow/actions/runs/20027925657/job/57429612781)|
|test-fedora-42-cpp|[](https://github.com/ursacomputing/crossbow/actions/runs/20027925646/job/57429612687)|
|test-ubuntu-22.04-cpp|[](https://github.com/ursacomputing/crossbow/actions/runs/20027926050/job/57429614120)|
|test-ubuntu-22.04-cpp-20|[](https://github.com/ursacomputing/crossbow/actions/runs/20027926139/job/57429625280)|
|test-ubuntu-22.04-cpp-bundled|[](https://github.com/ursacomputing/crossbow/actions/runs/20027926331/job/57429614867)|
|test-ubuntu-22.04-cpp-emscripten|[](https://github.com/ursacomputing/crossbow/actions/runs/20027926306/job/57429614773)|
|test-ubuntu-22.04-cpp-no-threading|[](https://github.com/ursacomputing/crossbow/actions/runs/20027926645/job/57429615742)|
|test-ubuntu-24.04-cpp|[](https://github.com/ursacomputing/crossbow/actions/runs/20027926804/job/57429616247)|
|test-ubuntu-24.04-cpp-bundled-offline|[](https://github.com/ursacomputing/crossbow/actions/runs/20027926044/job/57429613818)|
|test-ubuntu-24.04-cpp-gcc-13-bundled|[](https://github.com/ursacomputing/crossbow/actions/runs/20027926752/job/57429616045)|
|test-ubuntu-24.04-cpp-gcc-14|[](https://github.com/ursacomputing/crossbow/actions/runs/20027926097/job/57429613948)|
|test-ubuntu-24.04-cpp-minimal-with-formats|[](https://github.com/ursacomputing/crossbow/actions/runs/20027925864/job/57429613491)|
|test-ubuntu-24.04-cpp-thread-sanitizer|[](https://github.com/ursacomputing/crossbow/actions/runs/20027925873/job/57429613276)|
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-48123 [C++][Float16] Reimplement arrow::WithinUlp and Enable it for float16 [arrow]
Posted by "pitrou (via GitHub)" <gi...@apache.org>.
pitrou commented on code in PR #48224:
URL: https://github.com/apache/arrow/pull/48224#discussion_r2587954695
##########
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:
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.
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-48123 [C++][Float16] Reimplement arrow::WithinUlp and Enable it for float16 [arrow]
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on PR #48224:
URL: https://github.com/apache/arrow/pull/48224#issuecomment-3566428711
:warning: GitHub issue #48123 **has been automatically assigned in GitHub** to PR creator.
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-48123 [C++][Float16] Reimplement arrow::WithinUlp and Enable it for float16 [arrow]
Posted by "andishgar (via GitHub)" <gi...@apache.org>.
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-48123 [C++][Float16] Reimplement arrow::WithinUlp and Enable it for float16 [arrow]
Posted by "pitrou (via GitHub)" <gi...@apache.org>.
pitrou commented on PR #48224:
URL: https://github.com/apache/arrow/pull/48224#issuecomment-3626647214
@github-actions crossbow submit -g cpp
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-48123 [C++][Float16] Reimplement arrow::WithinUlp and Enable it for float16 [arrow]
Posted by "pitrou (via GitHub)" <gi...@apache.org>.
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-48123 [C++][Float16] Reimplement arrow::WithinUlp and Enable it for float16 [arrow]
Posted by "andishgar (via GitHub)" <gi...@apache.org>.
andishgar commented on PR #48224:
URL: https://github.com/apache/arrow/pull/48224#issuecomment-3582263978
@pitrou Thank you for the review. I’ve checked all the comments and will apply your suggestions in the coming days.
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-48123 [C++][Float16] Reimplement arrow::WithinUlp and Enable it for float16 [arrow]
Posted by "pitrou (via GitHub)" <gi...@apache.org>.
pitrou commented on code in PR #48224:
URL: https://github.com/apache/arrow/pull/48224#discussion_r2587950878
##########
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:
> 1-Is there any point in using this function? I mean, when the function is called, the value has already been loaded. This function is supposed to load the value byte by byte via memcpy, right?
In practice, the constant-size memcpy call will be optimized away by the compiler.
> 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?
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-48123 [C++][Float16] Reimplement arrow::WithinUlp and Enable it for float16 [arrow]
Posted by "andishgar (via GitHub)" <gi...@apache.org>.
andishgar commented on code in PR #48224:
URL: https://github.com/apache/arrow/pull/48224#discussion_r2569562687
##########
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 Regarding this:
I'm aware that the goal of the load functions in `arrow/util/ubsan` is to address the issue of unaligned loads on the x86-64 architecture. As far as I have checked, I should use the function below.
https://github.com/apache/arrow/blob/fa41b2674bfab627ec1c69b4b4fb52d9caff8c15/cpp/src/arrow/util/ubsan.h#L70-L79
However, two questions come to my mind:
1-Is there any point in using this function? I mean, when the function is called, the value has already been loaded. This function is supposed to load the value byte by byte via memcpy, right?
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?
Given this, is it okay to use this function? Keep in mind that this method is intended to be used in the next PR for the array. Additionally, this way of bit-casting is certainly much slower since it operates byte by byte.
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-48123 [C++][Float16] Reimplement arrow::WithinUlp and Enable it for float16 [arrow]
Posted by "andishgar (via GitHub)" <gi...@apache.org>.
andishgar commented on code in PR #48224:
URL: https://github.com/apache/arrow/pull/48224#discussion_r2587651401
##########
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
Would you consider `SafeCopy<T, U>` appropriate for unaligned loads of trivially copyable types in this context?
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-48123 [C++][Float16] Reimplement arrow::WithinUlp and Enable it for float16 [arrow]
Posted by "andishgar (via GitHub)" <gi...@apache.org>.
andishgar commented on code in PR #48224:
URL: https://github.com/apache/arrow/pull/48224#discussion_r2587659965
##########
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
Would you consider using` SafeCopy<T, U>` appropriate in this context?
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org