Skip to content

Commit

Permalink
[REVIEW] Remove fixed_point precise round (#6544)
Browse files Browse the repository at this point in the history 
* No roundf in division

* No shift_with_precise_round

* Update CHANGELOG

* Update cpp/tests/fixed_point/fixed_point_tests.cu

Co-authored-by: Mark Harris <[email protected]>

* Update cpp/tests/fixed_point/fixed_point_tests.cu

Co-authored-by: Mark Harris <[email protected]>

* Add missing CUDF_EXPECTS

Co-authored-by: Mark Harris <[email protected]>
  • Loading branch information
@codereport @harrism
codereport and harrism authored Oct 20, 2020
1 parent dd01d66 commit 71a188b
Show file tree
Hide file tree
Showing 4 changed files with 29 additions and 102 deletions.
1 change: 1 addition & 0 deletions CHANGELOG.md
View file
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,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
- PR #6555 Adapt JNI build to libcudf composition of multiple libraries

## Bug Fixes
Expand Down
84 changes: 4 additions & 80 deletions cpp/include/cudf/fixed_point/fixed_point.hpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -130,28 +130,6 @@ CUDA_HOST_DEVICE_CALLABLE constexpr T right_shift(T const& val, scale_type const
return val / ipow<Rep, Rad>(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 <typename Rep, Radix Rad, typename T>
CUDA_HOST_DEVICE_CALLABLE constexpr T right_shift_rounded(T const& val, scale_type const& scale)
{
Rep const factor = ipow<Rep, Rad>(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
Expand Down Expand Up @@ -193,59 +171,6 @@ CUDA_HOST_DEVICE_CALLABLE constexpr T shift(T const& val, scale_type const& scal
return left_shift<Rep, Rad>(val, scale);
}

/** @brief Function that performs precise shift to avoid "lossiness"
* inherent in floating point values
*
* Example: `auto n = fixed_point<int32_t, Radix::BASE_10>{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 <typename Rep,
Radix Rad,
typename T,
typename simt::std::enable_if_t<simt::std::is_integral<T>::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<Rep, Rad>(value, scale);
else
return left_shift<Rep, Rad>(value, scale);
}

/** @brief Function that performs precise shift to avoid "lossiness"
* inherent in floating point values
*
* Example: `auto n = fixed_point<int32_t, Radix::BASE_10>{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 <typename Rep,
Radix Rad,
typename T,
typename simt::std::enable_if_t<simt::std::is_floating_point<T>::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<int64_t, Rad>(std::abs(scale));
return std::roundf(scale <= 0 ? value * factor : value / factor);
}

} // namespace detail

/**
Expand Down Expand Up @@ -302,7 +227,7 @@ class fixed_point {
typename simt::std::enable_if_t<is_supported_construction_value_type<T>() &&
is_supported_representation_type<Rep>()>* = nullptr>
CUDA_HOST_DEVICE_CALLABLE explicit fixed_point(T const& value, scale_type const& scale)
: _value{detail::shift_with_precise_round<Rep, Rad>(value, scale)}, _scale{scale}
: _value{static_cast<Rep>(detail::shift<Rep, Rad>(value, scale))}, _scale{scale}
{
}

Expand Down Expand Up @@ -587,8 +512,7 @@ class fixed_point {
CUDA_HOST_DEVICE_CALLABLE fixed_point<Rep, Rad> rescaled(scale_type scale) const
{
if (scale == _scale) return *this;
Rep const value =
detail::shift_with_precise_round<Rep, Rad>(_value, scale_type{scale - _scale});
Rep const value = detail::shift<Rep, Rad>(_value, scale_type{scale - _scale});
return fixed_point<Rep, Rad>{scaled_integer<Rep>{value, scale}};
}
}; // namespace numeric
Expand Down Expand Up @@ -738,8 +662,8 @@ CUDA_HOST_DEVICE_CALLABLE fixed_point<Rep1, Rad1> operator/(fixed_point<Rep1, Ra

#endif

return fixed_point<Rep1, Rad1>{scaled_integer<Rep1>(std::roundf(lhs._value * 1.0 / rhs._value),
scale_type{lhs._scale - rhs._scale})};
return fixed_point<Rep1, Rad1>{
scaled_integer<Rep1>(lhs._value / rhs._value, scale_type{lhs._scale - rhs._scale})};
}

// EQUALITY COMPARISON Operation
Expand Down
2 changes: 2 additions & 0 deletions cpp/src/binaryop/binaryop.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -420,6 +420,7 @@ std::unique_ptr<column> 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");

Expand Down Expand Up @@ -495,6 +496,7 @@ std::unique_ptr<column> 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");

Expand Down
44 changes: 22 additions & 22 deletions cpp/tests/fixed_point/fixed_point_tests.cu
View file
Original file line number Diff line number Diff line change
Expand Up @@ -60,9 +60,9 @@ TYPED_TEST(FixedPointTestBothReps, SimpleDecimalXXConstruction)
EXPECT_EQ(1, static_cast<double>(num0));
EXPECT_EQ(1.2, static_cast<double>(num1));
EXPECT_EQ(1.23, static_cast<double>(num2));
EXPECT_EQ(1.235, static_cast<double>(num3)); // rounds up
EXPECT_EQ(1.2346, static_cast<double>(num4)); // rounds up
EXPECT_EQ(1.23457, static_cast<double>(num5)); // rounds up
EXPECT_EQ(1.234, static_cast<double>(num3));
EXPECT_EQ(1.2345, static_cast<double>(num4));
EXPECT_EQ(1.23456, static_cast<double>(num5));
EXPECT_EQ(1.234567, static_cast<double>(num6));
}

Expand All @@ -81,9 +81,9 @@ TYPED_TEST(FixedPointTestBothReps, SimpleNegativeDecimalXXConstruction)
EXPECT_EQ(-1, static_cast<double>(num0));
EXPECT_EQ(-1.2, static_cast<double>(num1));
EXPECT_EQ(-1.23, static_cast<double>(num2));
EXPECT_EQ(-1.235, static_cast<double>(num3)); // rounds up
EXPECT_EQ(-1.2346, static_cast<double>(num4)); // rounds up
EXPECT_EQ(-1.23457, static_cast<double>(num5)); // rounds up
EXPECT_EQ(-1.234, static_cast<double>(num3));
EXPECT_EQ(-1.2345, static_cast<double>(num4));
EXPECT_EQ(-1.23456, static_cast<double>(num5));
EXPECT_EQ(-1.234567, static_cast<double>(num6));
}

Expand All @@ -103,10 +103,10 @@ TYPED_TEST(FixedPointTestBothReps, PaddedDecimalXXConstruction)

EXPECT_EQ(1.1, static_cast<double>(a));
EXPECT_EQ(1.01, static_cast<double>(b));
EXPECT_EQ(1.001, static_cast<double>(c));
EXPECT_EQ(1, static_cast<double>(c)); // intentional (inherited problem from floating point)
EXPECT_EQ(1.0001, static_cast<double>(d));
EXPECT_EQ(1.00001, static_cast<double>(e));
EXPECT_EQ(1.000001, static_cast<double>(f));
EXPECT_EQ(1, static_cast<double>(f)); // intentional (inherited problem from floating point)

EXPECT_TRUE(1.000123 - static_cast<double>(x) < std::numeric_limits<double>::epsilon());
EXPECT_EQ(0.000123, static_cast<double>(y));
Expand All @@ -130,9 +130,9 @@ TYPED_TEST(FixedPointTestBothReps, SimpleBinaryFPConstruction)

EXPECT_EQ(10, static_cast<double>(num0));
EXPECT_EQ(10, static_cast<double>(num1));
EXPECT_EQ(12, static_cast<double>(num2));
EXPECT_EQ(8, static_cast<double>(num2));
EXPECT_EQ(8, static_cast<double>(num3));
EXPECT_EQ(16, static_cast<double>(num4));
EXPECT_EQ(0, static_cast<double>(num4));

EXPECT_EQ(1, static_cast<double>(num5));
EXPECT_EQ(1, static_cast<double>(num6));
Expand All @@ -149,7 +149,7 @@ TYPED_TEST(FixedPointTestBothReps, MoreSimpleBinaryFPConstruction)
binary_fp num1{2.1, scale_type{-4}};

EXPECT_EQ(1.25, static_cast<double>(num0));
EXPECT_EQ(2.125, static_cast<double>(num1));
EXPECT_EQ(2.0625, static_cast<double>(num1));
}

TYPED_TEST(FixedPointTestBothReps, SimpleDecimalXXMath)
Expand Down Expand Up @@ -207,23 +207,23 @@ TYPED_TEST(FixedPointTestBothReps, DecimalXXTrickyDivision)
decimalXX TEN_1{10, scale_type{1}};
decimalXX SIXTY_1{60, scale_type{1}};

EXPECT_EQ(static_cast<int32_t>(ONE_1), 0); // round(1 / 10) = 0
EXPECT_EQ(static_cast<int32_t>(SIX_1), 10); // round(6 / 10) = 10
EXPECT_EQ(static_cast<int32_t>(ONE_1), 0);
EXPECT_EQ(static_cast<int32_t>(SIX_1), 0);
EXPECT_EQ(static_cast<int32_t>(TEN_0), 10);
EXPECT_EQ(static_cast<int32_t>(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<int32_t>(A / B), 30);
EXPECT_EQ(static_cast<int32_t>(A / B), 20);
EXPECT_EQ(static_cast<int32_t>((A * C) / B), 28);

decimalXX n{28, scale_type{1}};
EXPECT_EQ(static_cast<int32_t>(n), 30);
EXPECT_EQ(static_cast<int32_t>(n), 20);
}

TYPED_TEST(FixedPointTestBothReps, DecimalXXRounding)
Expand All @@ -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}};
Expand Down Expand Up @@ -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}));
Expand All @@ -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<TypeParam>(num0.rescaled(scale_type{3})));
EXPECT_EQ(1000, static_cast<TypeParam>(num0.rescaled(scale_type{3})));
EXPECT_EQ(1000, static_cast<TypeParam>(num1.rescaled(scale_type{3})));
EXPECT_EQ(-1000, static_cast<TypeParam>(num2.rescaled(scale_type{3})));
EXPECT_EQ(-2000, static_cast<TypeParam>(num3.rescaled(scale_type{3})));
EXPECT_EQ(-1000, static_cast<TypeParam>(num3.rescaled(scale_type{3})));
}

TYPED_TEST(FixedPointTestBothReps, DecimalXXThrust)
Expand Down

0 comments on commit 71a188b

Please sign in to comment.