Fix inaccurate ceil/floor and inaccurate rescaling casts of fixed-point values. #14242
Conversation
|
Why in a subsequent release? Why not just fix them now? |
This PR is not ready for review yet, I'm still moving things around. I'm targeting a fix for the problems with floating precision loss in fixed point operations and tests that catch these bugs for 23.10. Those fixes and tests will go in this PR. The refactoring of intpow operations across libcudf will target 23.12 after this PR and #14233 are forward-merged. |
bdice
changed the title
bdice
changed the title
bdice
force-pushed
the
decimal128-rescaling-errors
branch
from
37e79f4
to
1fc7fb5
Compare
bdice
requested review from
hyperbolic2346 and
vuule
and removed request for
a team
| @@ -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; | |||
There was a problem hiding this comment.
Is the next step to consolidate this logic into a single function somewhere?
There was a problem hiding this comment.
As @hyperbolic2346 implied, it would be nice to consolidate logic into a util function, but the fix looks good.
|
|
||
| Type n = 10; | ||
| for (int i = 1; i < -input.type().scale(); ++i) { | ||
| n *= 10; |
There was a problem hiding this comment.
do we need to worry about overflow?
There was a problem hiding this comment.
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).
Description
This is a follow-up PR to #14233. This PR fixes a bug where floating-point values were used as intermediates in ceil/floor unary operations and cast operations that require rescaling for fixed-point types, giving inaccurate results.
See also:
Checklist