From 2c7c266ab580df12fa548e8083b81a0012f0ab21 Mon Sep 17 00:00:00 2001 From: Conor Hoekstra Date: Thu, 15 Oct 2020 17:22:17 -0400 Subject: [PATCH 1/6] No roundf in division --- cpp/include/cudf/fixed_point/fixed_point.hpp | 4 ++-- cpp/tests/fixed_point/fixed_point_tests.cu | 4 ++-- 2 files changed, 4 insertions(+), 4 deletions(-) diff --git a/cpp/include/cudf/fixed_point/fixed_point.hpp b/cpp/include/cudf/fixed_point/fixed_point.hpp index d45ee21a250..a6f54629bb7 100644 --- a/cpp/include/cudf/fixed_point/fixed_point.hpp +++ b/cpp/include/cudf/fixed_point/fixed_point.hpp @@ -738,8 +738,8 @@ CUDA_HOST_DEVICE_CALLABLE fixed_point operator/(fixed_point{scaled_integer(std::roundf(lhs._value * 1.0 / rhs._value), - scale_type{lhs._scale - rhs._scale})}; + return fixed_point{ + scaled_integer(lhs._value / rhs._value, scale_type{lhs._scale - rhs._scale})}; } // EQUALITY COMPARISON Operation diff --git a/cpp/tests/fixed_point/fixed_point_tests.cu b/cpp/tests/fixed_point/fixed_point_tests.cu index a1ccab22356..10f25182bea 100644 --- a/cpp/tests/fixed_point/fixed_point_tests.cu +++ b/cpp/tests/fixed_point/fixed_point_tests.cu @@ -212,14 +212,14 @@ TYPED_TEST(FixedPointTestBothReps, DecimalXXTrickyDivision) EXPECT_EQ(static_cast(TEN_0), 10); EXPECT_EQ(static_cast(SIXTY_1), 60); - EXPECT_EQ(SIXTY_1 / TEN_0, TEN_1); + EXPECT_EQ(SIXTY_1 / TEN_0, ONE_1); EXPECT_EQ(SIXTY_1 / TEN_1, SIX_0); decimalXX A{34.56, scale_type{-2}}; decimalXX B{1.234, scale_type{-3}}; decimalXX C{1, scale_type{-2}}; - EXPECT_EQ(static_cast(A / B), 30); + EXPECT_EQ(static_cast(A / B), 20); EXPECT_EQ(static_cast((A * C) / B), 28); decimalXX n{28, scale_type{1}}; From 866fa282aafbbf2ae800a2a55a07b905ee34c092 Mon Sep 17 00:00:00 2001 From: Conor Hoekstra Date: Thu, 15 Oct 2020 19:28:44 -0400 Subject: [PATCH 2/6] No shift_with_precise_round --- cpp/include/cudf/fixed_point/fixed_point.hpp | 80 +------------------- cpp/tests/fixed_point/fixed_point_tests.cu | 40 +++++----- 2 files changed, 22 insertions(+), 98 deletions(-) diff --git a/cpp/include/cudf/fixed_point/fixed_point.hpp b/cpp/include/cudf/fixed_point/fixed_point.hpp index a6f54629bb7..4d33107417a 100644 --- a/cpp/include/cudf/fixed_point/fixed_point.hpp +++ b/cpp/include/cudf/fixed_point/fixed_point.hpp @@ -130,28 +130,6 @@ CUDA_HOST_DEVICE_CALLABLE constexpr T right_shift(T const& val, scale_type const return val / ipow(scale._t); } -/** @brief Function that performs a rounding `right shift` scale "times" on the `val` - * - * The scaled integer equivalent of 0.5 is added to the value before truncating such that - * any remaining fractional part will be rounded away from zero. - * - * Note: perform this operation when constructing with positive scale - * - * @tparam Rep Representation type needed for integer exponentiation - * @tparam Rad The radix which will act as the base in the exponentiation - * @tparam T Type for value `val` being shifted and the return type - * @param val The value being shifted - * @param scale The amount to shift the value by - * @return Shifted value of type T - */ -template -CUDA_HOST_DEVICE_CALLABLE constexpr T right_shift_rounded(T const& val, scale_type const& scale) -{ - Rep const factor = ipow(scale._t); - Rep const half = factor / 2; - return (val >= 0 ? val + half : val - half) / factor; -} - /** @brief Function that performs a `left shift` scale "times" on the `val` * * Note: perform this operation when constructing with negative scale @@ -193,59 +171,6 @@ CUDA_HOST_DEVICE_CALLABLE constexpr T shift(T const& val, scale_type const& scal return left_shift(val, scale); } -/** @brief Function that performs precise shift to avoid "lossiness" - * inherent in floating point values - * - * Example: `auto n = fixed_point{1.001, scale_type{-3}}` - * will construct n to have a value of 1 without the precise shift - * - * @tparam Rep Representation type needed for integer exponentiation - * @tparam Rad The radix which will act as the base in the exponentiation - * @tparam T Type for value `val` being shifted and the return type - * @param value The value being shifted - * @param scale The amount to shift the value by - * @return Shifted value of type T - */ -template ::value>* = nullptr> -CUDA_HOST_DEVICE_CALLABLE auto shift_with_precise_round(T const& value, scale_type const& scale) - -> Rep -{ - if (scale == 0) - return value; - else if (scale > 0) - return right_shift_rounded(value, scale); - else - return left_shift(value, scale); -} - -/** @brief Function that performs precise shift to avoid "lossiness" - * inherent in floating point values - * - * Example: `auto n = fixed_point{1.001, scale_type{-3}}` - * will construct n to have a value of 1 without the precise shift - * - * @tparam Rep Representation type needed for integer exponentiation - * @tparam Rad The radix which will act as the base in the exponentiation - * @tparam T Type for value `val` being shifted and the return type - * @param value The value being shifted - * @param scale The amount to shift the value by - * @return Shifted value of type T - */ -template ::value>* = nullptr> -CUDA_HOST_DEVICE_CALLABLE auto shift_with_precise_round(T const& value, scale_type const& scale) - -> Rep -{ - if (scale == 0) return value; - T const factor = ipow(std::abs(scale)); - return std::roundf(scale <= 0 ? value * factor : value / factor); -} - } // namespace detail /** @@ -302,7 +227,7 @@ class fixed_point { typename simt::std::enable_if_t() && is_supported_representation_type()>* = nullptr> CUDA_HOST_DEVICE_CALLABLE explicit fixed_point(T const& value, scale_type const& scale) - : _value{detail::shift_with_precise_round(value, scale)}, _scale{scale} + : _value{static_cast(detail::shift(value, scale))}, _scale{scale} { } @@ -587,8 +512,7 @@ class fixed_point { CUDA_HOST_DEVICE_CALLABLE fixed_point rescaled(scale_type scale) const { if (scale == _scale) return *this; - Rep const value = - detail::shift_with_precise_round(_value, scale_type{scale - _scale}); + Rep const value = detail::shift(_value, scale_type{scale - _scale}); return fixed_point{scaled_integer{value, scale}}; } }; // namespace numeric diff --git a/cpp/tests/fixed_point/fixed_point_tests.cu b/cpp/tests/fixed_point/fixed_point_tests.cu index 10f25182bea..1afd87b1d16 100644 --- a/cpp/tests/fixed_point/fixed_point_tests.cu +++ b/cpp/tests/fixed_point/fixed_point_tests.cu @@ -60,9 +60,9 @@ TYPED_TEST(FixedPointTestBothReps, SimpleDecimalXXConstruction) EXPECT_EQ(1, static_cast(num0)); EXPECT_EQ(1.2, static_cast(num1)); EXPECT_EQ(1.23, static_cast(num2)); - EXPECT_EQ(1.235, static_cast(num3)); // rounds up - EXPECT_EQ(1.2346, static_cast(num4)); // rounds up - EXPECT_EQ(1.23457, static_cast(num5)); // rounds up + EXPECT_EQ(1.234, static_cast(num3)); + EXPECT_EQ(1.2345, static_cast(num4)); + EXPECT_EQ(1.23456, static_cast(num5)); EXPECT_EQ(1.234567, static_cast(num6)); } @@ -81,9 +81,9 @@ TYPED_TEST(FixedPointTestBothReps, SimpleNegativeDecimalXXConstruction) EXPECT_EQ(-1, static_cast(num0)); EXPECT_EQ(-1.2, static_cast(num1)); EXPECT_EQ(-1.23, static_cast(num2)); - EXPECT_EQ(-1.235, static_cast(num3)); // rounds up - EXPECT_EQ(-1.2346, static_cast(num4)); // rounds up - EXPECT_EQ(-1.23457, static_cast(num5)); // rounds up + EXPECT_EQ(-1.234, static_cast(num3)); + EXPECT_EQ(-1.2345, static_cast(num4)); + EXPECT_EQ(-1.23456, static_cast(num5)); EXPECT_EQ(-1.234567, static_cast(num6)); } @@ -103,10 +103,10 @@ TYPED_TEST(FixedPointTestBothReps, PaddedDecimalXXConstruction) EXPECT_EQ(1.1, static_cast(a)); EXPECT_EQ(1.01, static_cast(b)); - EXPECT_EQ(1.001, static_cast(c)); + EXPECT_EQ(1, static_cast(c)); // intentional (inhereted problem from floating point) EXPECT_EQ(1.0001, static_cast(d)); EXPECT_EQ(1.00001, static_cast(e)); - EXPECT_EQ(1.000001, static_cast(f)); + EXPECT_EQ(1, static_cast(f)); // intentional (inhereted problem from floating point) EXPECT_TRUE(1.000123 - static_cast(x) < std::numeric_limits::epsilon()); EXPECT_EQ(0.000123, static_cast(y)); @@ -130,9 +130,9 @@ TYPED_TEST(FixedPointTestBothReps, SimpleBinaryFPConstruction) EXPECT_EQ(10, static_cast(num0)); EXPECT_EQ(10, static_cast(num1)); - EXPECT_EQ(12, static_cast(num2)); + EXPECT_EQ(8, static_cast(num2)); EXPECT_EQ(8, static_cast(num3)); - EXPECT_EQ(16, static_cast(num4)); + EXPECT_EQ(0, static_cast(num4)); EXPECT_EQ(1, static_cast(num5)); EXPECT_EQ(1, static_cast(num6)); @@ -149,7 +149,7 @@ TYPED_TEST(FixedPointTestBothReps, MoreSimpleBinaryFPConstruction) binary_fp num1{2.1, scale_type{-4}}; EXPECT_EQ(1.25, static_cast(num0)); - EXPECT_EQ(2.125, static_cast(num1)); + EXPECT_EQ(2.0625, static_cast(num1)); } TYPED_TEST(FixedPointTestBothReps, SimpleDecimalXXMath) @@ -207,8 +207,8 @@ TYPED_TEST(FixedPointTestBothReps, DecimalXXTrickyDivision) decimalXX TEN_1{10, scale_type{1}}; decimalXX SIXTY_1{60, scale_type{1}}; - EXPECT_EQ(static_cast(ONE_1), 0); // round(1 / 10) = 0 - EXPECT_EQ(static_cast(SIX_1), 10); // round(6 / 10) = 10 + EXPECT_EQ(static_cast(ONE_1), 0); + EXPECT_EQ(static_cast(SIX_1), 0); EXPECT_EQ(static_cast(TEN_0), 10); EXPECT_EQ(static_cast(SIXTY_1), 60); @@ -223,7 +223,7 @@ TYPED_TEST(FixedPointTestBothReps, DecimalXXTrickyDivision) EXPECT_EQ(static_cast((A * C) / B), 28); decimalXX n{28, scale_type{1}}; - EXPECT_EQ(static_cast(n), 30); + EXPECT_EQ(static_cast(n), 20); } TYPED_TEST(FixedPointTestBothReps, DecimalXXRounding) @@ -236,7 +236,7 @@ TYPED_TEST(FixedPointTestBothReps, DecimalXXRounding) decimalXX FOUR_0{4, scale_type{0}}; decimalXX FIVE_0{5, scale_type{0}}; decimalXX TEN_0{10, scale_type{0}}; - decimalXX TEN_1{5, scale_type{1}}; + decimalXX TEN_1{10, scale_type{1}}; decimalXX FOURTEEN_0{14, scale_type{0}}; decimalXX FIFTEEN_0{15, scale_type{0}}; @@ -286,9 +286,9 @@ TYPED_TEST(FixedPointTestBothReps, RescaledTest) decimalXX num0{1, scale_type{0}}; decimalXX num1{1.2, scale_type{-1}}; decimalXX num2{1.23, scale_type{-2}}; - decimalXX num3{1.235, scale_type{-3}}; - decimalXX num4{1.2346, scale_type{-4}}; - decimalXX num5{1.23457, scale_type{-5}}; + decimalXX num3{1.234, scale_type{-3}}; + decimalXX num4{1.2345, scale_type{-4}}; + decimalXX num5{1.23456, scale_type{-5}}; decimalXX num6{1.234567, scale_type{-6}}; EXPECT_EQ(num0, num6.rescaled(scale_type{0})); @@ -308,10 +308,10 @@ TYPED_TEST(FixedPointTestBothReps, RescaledRounding) decimalXX num2{-1499, scale_type{0}}; decimalXX num3{-1500, scale_type{0}}; - EXPECT_EQ(2000, static_cast(num0.rescaled(scale_type{3}))); + EXPECT_EQ(1000, static_cast(num0.rescaled(scale_type{3}))); EXPECT_EQ(1000, static_cast(num1.rescaled(scale_type{3}))); EXPECT_EQ(-1000, static_cast(num2.rescaled(scale_type{3}))); - EXPECT_EQ(-2000, static_cast(num3.rescaled(scale_type{3}))); + EXPECT_EQ(-1000, static_cast(num3.rescaled(scale_type{3}))); } TYPED_TEST(FixedPointTestBothReps, DecimalXXThrust) From 8fce5ddec8e6a4c969c1e590e80ceef049d88648 Mon Sep 17 00:00:00 2001 From: Conor Hoekstra Date: Thu, 15 Oct 2020 19:39:04 -0400 Subject: [PATCH 3/6] Update CHANGELOG --- CHANGELOG.md | 1 + 1 file changed, 1 insertion(+) diff --git a/CHANGELOG.md b/CHANGELOG.md index 90ad29a2e02..3487c2204f3 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -13,6 +13,7 @@ - PR #6485 Add File IO to cuIO benchmarks - PR #6504 Update Java bindings version to 0.17-SNAPSHOT - PR #6527 Refactor DeviceColumnViewAccess to avoid JNI returning an array +- PR #6544 Remove `fixed_point` precise round ## Bug Fixes - PR #6506 Fix DateTime type value truncation while writing to csv From 29814d37d8473c7e4be2a34dc3c464004d2acd2b Mon Sep 17 00:00:00 2001 From: Conor Hoekstra <36027403+codereport@users.noreply.github.com> Date: Mon, 19 Oct 2020 21:37:18 -0400 Subject: [PATCH 4/6] Update cpp/tests/fixed_point/fixed_point_tests.cu Co-authored-by: Mark Harris --- cpp/tests/fixed_point/fixed_point_tests.cu | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/cpp/tests/fixed_point/fixed_point_tests.cu b/cpp/tests/fixed_point/fixed_point_tests.cu index 1afd87b1d16..52987e6957d 100644 --- a/cpp/tests/fixed_point/fixed_point_tests.cu +++ b/cpp/tests/fixed_point/fixed_point_tests.cu @@ -106,7 +106,7 @@ TYPED_TEST(FixedPointTestBothReps, PaddedDecimalXXConstruction) EXPECT_EQ(1, static_cast(c)); // intentional (inhereted problem from floating point) EXPECT_EQ(1.0001, static_cast(d)); EXPECT_EQ(1.00001, static_cast(e)); - EXPECT_EQ(1, static_cast(f)); // intentional (inhereted problem from floating point) + EXPECT_EQ(1, static_cast(f)); // intentional (inherited problem from floating point) EXPECT_TRUE(1.000123 - static_cast(x) < std::numeric_limits::epsilon()); EXPECT_EQ(0.000123, static_cast(y)); From 8d1deb4b7f3eaaf101b4e03964658e328c385db5 Mon Sep 17 00:00:00 2001 From: Conor Hoekstra <36027403+codereport@users.noreply.github.com> Date: Mon, 19 Oct 2020 21:37:28 -0400 Subject: [PATCH 5/6] Update cpp/tests/fixed_point/fixed_point_tests.cu Co-authored-by: Mark Harris --- cpp/tests/fixed_point/fixed_point_tests.cu | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/cpp/tests/fixed_point/fixed_point_tests.cu b/cpp/tests/fixed_point/fixed_point_tests.cu index 52987e6957d..e1fe7c7f1e3 100644 --- a/cpp/tests/fixed_point/fixed_point_tests.cu +++ b/cpp/tests/fixed_point/fixed_point_tests.cu @@ -103,7 +103,7 @@ TYPED_TEST(FixedPointTestBothReps, PaddedDecimalXXConstruction) EXPECT_EQ(1.1, static_cast(a)); EXPECT_EQ(1.01, static_cast(b)); - EXPECT_EQ(1, static_cast(c)); // intentional (inhereted problem from floating point) + EXPECT_EQ(1, static_cast(c)); // intentional (inherited problem from floating point) EXPECT_EQ(1.0001, static_cast(d)); EXPECT_EQ(1.00001, static_cast(e)); EXPECT_EQ(1, static_cast(f)); // intentional (inherited problem from floating point) From 4f647d1ff38b5e7815c059b8e917216d8b3734b5 Mon Sep 17 00:00:00 2001 From: Conor Hoekstra Date: Mon, 19 Oct 2020 21:48:56 -0400 Subject: [PATCH 6/6] Add missing CUDF_EXPECTS --- cpp/src/binaryop/binaryop.cpp | 2 ++ 1 file changed, 2 insertions(+) diff --git a/cpp/src/binaryop/binaryop.cpp b/cpp/src/binaryop/binaryop.cpp index 16b56ad86e7..9d8cdf1e610 100644 --- a/cpp/src/binaryop/binaryop.cpp +++ b/cpp/src/binaryop/binaryop.cpp @@ -420,6 +420,7 @@ std::unique_ptr fixed_point_binary_operation(scalar const& lhs, { using namespace numeric; + CUDF_EXPECTS(is_supported_fixed_point_binop(op), "Unsupported fixed_point binary operation"); CUDF_EXPECTS(lhs.type().id() == rhs.type().id(), "Both columns must be of the same fixed_point type"); @@ -495,6 +496,7 @@ std::unique_ptr fixed_point_binary_operation(column_view const& lhs, { using namespace numeric; + CUDF_EXPECTS(is_supported_fixed_point_binop(op), "Unsupported fixed_point binary operation"); CUDF_EXPECTS(lhs.type().id() == rhs.type().id(), "Both columns must be of the same fixed_point type");