Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix inaccurate ceil/floor and inaccurate rescaling casts of fixed-point values. #14242

Merged
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 7 additions & 1 deletion cpp/src/unary/cast_ops.cu
Original file line number Diff line number Diff line change
Expand Up @@ -199,7 +199,13 @@ std::unique_ptr<column> rescale(column_view input,
}
return output_column;
}
auto const scalar = make_fixed_point_scalar<T>(std::pow(10, -diff), scale_type{diff}, stream);

RepType scalar_value = 10;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is the next step to consolidate this logic into a single function somewhere?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes. See #14243.

for (int i = 1; i < -diff; ++i) {
scalar_value *= 10;
}

auto const scalar = make_fixed_point_scalar<T>(scalar_value, scale_type{diff}, stream);
return detail::binary_operation(input, *scalar, binary_operator::DIV, type, stream, mr);
}
};
Expand Down
8 changes: 6 additions & 2 deletions cpp/src/unary/math_ops.cu
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
/*
* Copyright (c) 2019-2022, NVIDIA CORPORATION.
* Copyright (c) 2019-2023, NVIDIA CORPORATION.
*
* Licensed under the Apache License, Version 2.0 (the "License");
* you may not use this file except in compliance with the License.
Expand Down Expand Up @@ -295,7 +295,11 @@ std::unique_ptr<column> unary_op_with(column_view const& input,
input.type(), input.size(), copy_bitmask(input, stream, mr), input.null_count(), stream, mr);

auto out_view = result->mutable_view();
Type const n = std::pow(10, -input.type().scale());

Type n = 10;
for (int i = 1; i < -input.type().scale(); ++i) {
n *= 10;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

do we need to worry about overflow?

Copy link
Contributor Author

@bdice bdice Oct 3, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No, I don't think so. We know that the value 10^-scale is representable by the device storage type (e.g. __int128_t or int64_t). And in the worst case, this won't introduce a new overflow if one already existed, because the value was already being converted to that type (it was just an inaccurate value).

}

thrust::transform(rmm::exec_policy(stream),
input.begin<Type>(),
Expand Down
40 changes: 40 additions & 0 deletions cpp/tests/unary/cast_tests.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -30,6 +30,8 @@
#include <thrust/host_vector.h>
#include <thrust/iterator/counting_iterator.h>

#include <cuda/std/limits>

#include <type_traits>
#include <vector>

Expand Down Expand Up @@ -967,6 +969,44 @@ TYPED_TEST(FixedPointTests, Decimal128ToDecimalXXWithLargerScale)
CUDF_TEST_EXPECT_COLUMNS_EQUAL(expected, result->view());
}

TYPED_TEST(FixedPointTests, ValidateCastRescalePrecision)
{
using namespace numeric;
using decimalXX = TypeParam;
using RepType = cudf::device_storage_type_t<decimalXX>;
using fp_wrapper = cudf::test::fixed_point_column_wrapper<RepType>;

// This test is designed to protect against floating point conversion
// introducing errors in fixed-point arithmetic. The rescaling that occurs
// during casting to different scales should only use fixed-precision math.
// Realistically, we are only able to show precision failures due to floating
// conversion in a few very specific circumstances where dividing by specific
// powers of 10 works against us. Some examples: 10^23, 10^25, 10^26, 10^27,
// 10^30, 10^32, 10^36. See https://godbolt.org/z/cP1MddP8P for a derivation.
// For completeness and to ensure that we are not missing any other cases, we
// test casting to/from all scales in the range of each decimal type. Values
// that are powers of ten show this error more readily than non-powers of 10
// because the rescaling factor is a power of 10, meaning that errors in
// division are more visible.
constexpr auto min_scale = -cuda::std::numeric_limits<RepType>::digits10;
for (int input_scale = 0; input_scale >= min_scale; --input_scale) {
for (int result_scale = 0; result_scale >= min_scale; --result_scale) {
RepType input_value = 1;
for (int k = 0; k > input_scale; --k) {
input_value *= 10;
}
RepType result_value = 1;
for (int k = 0; k > result_scale; --k) {
result_value *= 10;
}
auto const input = fp_wrapper{{input_value}, scale_type{input_scale}};
auto const expected = fp_wrapper{{result_value}, scale_type{result_scale}};
auto const result = cudf::cast(input, make_fixed_point_data_type<decimalXX>(result_scale));
CUDF_TEST_EXPECT_COLUMNS_EQUAL(expected, result->view());
}
}
}

TYPED_TEST(FixedPointTests, Decimal32ToDecimalXXWithLargerScaleAndNullMask)
{
using namespace numeric;
Expand Down
33 changes: 33 additions & 0 deletions cpp/tests/unary/unary_ops_test.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -24,6 +24,8 @@

#include <thrust/iterator/counting_iterator.h>

#include <cuda/std/limits>

template <typename T>
cudf::test::fixed_width_column_wrapper<T> create_fixed_columns(cudf::size_type start,
cudf::size_type size,
Expand Down Expand Up @@ -372,4 +374,35 @@ TYPED_TEST(FixedPointUnaryTests, FixedPointUnaryFloorLarge)
CUDF_TEST_EXPECT_COLUMNS_EQUAL(expected, result->view());
}

TYPED_TEST(FixedPointUnaryTests, ValidateCeilFloorPrecision)
{
using namespace numeric;
using decimalXX = TypeParam;
using RepType = cudf::device_storage_type_t<decimalXX>;
using fp_wrapper = cudf::test::fixed_point_column_wrapper<RepType>;

// This test is designed to protect against floating point conversion
// introducing errors in fixed-point arithmetic. The rounding that occurs
// during ceil/floor should only use fixed-precision math. Realistically,
// we are only able to show precision failures due to floating conversion in
// a few very specific circumstances where dividing by specific powers of 10
// works against us. Some examples: 10^23, 10^25, 10^26, 10^27, 10^30,
// 10^32, 10^36. See https://godbolt.org/z/cP1MddP8P for a derivation. For
// completeness and to ensure that we are not missing any other cases, we
// test all scales representable in the range of each decimal type.
constexpr auto min_scale = -cuda::std::numeric_limits<RepType>::digits10;
for (int input_scale = 0; input_scale >= min_scale; --input_scale) {
RepType input_value = 1;
for (int k = 0; k > input_scale; --k) {
input_value *= 10;
}
auto const input = fp_wrapper{{input_value}, scale_type{input_scale}};
auto const expected = fp_wrapper{{input_value}, scale_type{input_scale}};
auto const ceil_result = cudf::unary_operation(input, cudf::unary_operator::CEIL);
CUDF_TEST_EXPECT_COLUMNS_EQUAL(expected, ceil_result->view());
auto const floor_result = cudf::unary_operation(input, cudf::unary_operator::FLOOR);
CUDF_TEST_EXPECT_COLUMNS_EQUAL(expected, floor_result->view());
}
}

CUDF_TEST_PROGRAM_MAIN()