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 inaccuracy in decimal128 rounding. #14233

Merged
merged 3 commits into from
Oct 3, 2023

Conversation

bdice
Copy link
Contributor

@bdice bdice commented Sep 28, 2023

Description

Fixes a bug where floating-point values were used in decimal128 rounding, giving wrong results.

Closes #14210.

Checklist

  • I am familiar with the Contributing Guidelines.
  • New or existing tests cover these changes.
  • The documentation is up to date with these changes.

@github-actions github-actions bot added the libcudf Affects libcudf (C++/CUDA) code. label Sep 28, 2023
@@ -271,7 +271,10 @@ std::unique_ptr<column> round_with(column_view const& input,
out_view.template end<Type>(),
static_cast<Type>(0));
} else {
Type const n = std::pow(10, scale_movement);
Type n = 10;
for (int i = 1; i < scale_movement; ++i) {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This should use exponentiation-by-squaring for efficiency, and we should have a common implementation of that in libcudf. (We already have two implementations of exponentiation-by-squaring.) I have started work on this and will push to this PR when it's ready, but that might not happen today.

Copy link
Contributor

Choose a reason for hiding this comment

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

Awesome Bradley.
Would you point to the other two implementations? I was trying to look for them myself earlier today.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

One is in fixed point code and has a base that is known as a template parameter:

CUDF_HOST_DEVICE inline Rep ipow(T exponent)

The other is in binary ops code and its base and exponent are both runtime parameters:

I am not sure how to best refactor this, but I have drafted some work locally (not yet pushed) that would add a file cudf/detail/utilities/intpow.hpp that centralizes this logic and exposes both a "constexpr base" and "runtime base" form of the function. I'll push this soon so it can be evaluated -- but there's some hangups I am seeing locally with include order and cuda_runtime.h macro conflicts (__forceinline__) with CCCL (resolved in libcudacxx 2.2.0).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Note that we do not have an intpow AST operator, because one has not been requested (to the best of my knowledge). It would go somewhere in here, but would need to have a different name like INTPOW to disambiguate it from the operators that are expected to return floating point values:

struct operator_functor<ast_operator::POW, false> {

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The topic of integer powers was heavily discussed and analyzed in #10178.

Copy link
Contributor Author

@bdice bdice Sep 29, 2023

Choose a reason for hiding this comment

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

There are two more places where I think this bug might reoccur:

I'd love help writing some tests that fail for these cases.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm starting work on a follow-up PR to fix these additional rescaling issues in #14242. I have a checklist there. This PR should be limited in scope to fixing only the rounding issues, to minimize friction for this fix. I'd like to target refactoring requests to #14242 (aiming for 23.10) or a subsequent release (probably 23.12)

Copy link
Contributor

Choose a reason for hiding this comment

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

See also #9346

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I opened #14243 to track future work on this.

@bdice bdice changed the title Add failing tests for decimal128 rounding. Fix inaccuracy in decimal128 rounding. Sep 28, 2023
@bdice bdice added bug Something isn't working non-breaking Non-breaking change labels Sep 28, 2023
@bdice bdice self-assigned this Sep 28, 2023
@bdice bdice marked this pull request as ready for review October 2, 2023 21:29
@bdice bdice requested a review from a team as a code owner October 2, 2023 21:29
@bdice bdice requested review from harrism and divyegala and removed request for a team October 2, 2023 21:29
@galipremsagar
Copy link
Contributor

@bdice does this PR also fix this issue: #14169

@bdice
Copy link
Contributor Author

bdice commented Oct 3, 2023

@bdice does this PR also fix this issue: #14169

No, I don't think it does from a quick local test. I also tried #14242, which I hope handles a similar bug in rescaling operations. Could you please add more information to #14169 about the root cause and how it was fixed in Arrow? Then we can investigate that after handling this bug and #14242.

Copy link
Member

@harrism harrism left a comment

Choose a reason for hiding this comment

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

I don't really like the ITM and loop. But you say the refactoring is elsewhere, so I hope you will use something like Jake's approach instead.

@bdice
Copy link
Contributor Author

bdice commented Oct 3, 2023

I don't really like the ITM and loop. But you say the refactoring is elsewhere, so I hope you will use something like Jake's approach instead.

Right -- I definitely don't intend to keep this as a runtime loop. We should be using a lookup table or, if outside the bounds of the lookup, exponentiation-by-squaring. I just don't want to introduce that in this PR, to keep the diff minimal since we're in code freeze for 23.10. I am planning to fix #14242 in the same way as this PR, and then refactor the several places where int-pow operations occur in libcudf in a separate PR (a draft of an int-pow refactored header is also in #14242 for the moment, but I will pull it out into a separate PR before merging that into the frozen 23.10 branch).

@raydouglass raydouglass merged commit 66a655c into rapidsai:branch-23.10 Oct 3, 2023
59 of 60 checks passed
raydouglass pushed a commit that referenced this pull request Oct 3, 2023
…nt values. (#14242)

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:
- #14233 (comment)
- #14243

Authors:
   - Bradley Dice (https://github.com/bdice)

Approvers:
   - Mike Wilson (https://github.com/hyperbolic2346)
   - Vukasin Milovanovic (https://github.com/vuule)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working libcudf Affects libcudf (C++/CUDA) code. non-breaking Non-breaking change
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

7 participants