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

added more testing of value_cast conversion accuracy when using integral types #579

Merged

Conversation

burnpanck
Copy link
Contributor

I had some doubts regarding the number * val(num) / val(den) * val(irr) computation path in sudo_cast<Q>, where I see two potential risks:

  1. Potential overflow in the first multiplication (* val(num))
  2. Truncation in the division: It appears that / val(den) * val(irr) may lead to more truncation than necessary (i.e. compared to * val(irr) / val(den)), depending on the exact values there.

However, so far, I was unable to find an actual edge case. Risk 1 seems to be mitigated through the use of std::intmax_t as the intermediate type of computation,. Risk 2 appears to be mitigated by selecting the other (floating-point) computation path whenever irrational factors are involved.

So instead of a bug report, this is now just a PR containing the additional test-cases I used to verify that those issues are indeed not present. I believe there is value in keeping those in the code-base, in case the implementation of sudo_cast is touched for some reason or another in the future. For example, std::intmax_t may be excessive if both input ant output representation are in fact quite small integers.

@burnpanck burnpanck marked this pull request as ready for review June 2, 2024 09:24
@burnpanck
Copy link
Contributor Author

Actually, I still believe there is the potential for overflow even in the current implementation, I just need to craft a suitable test-case. The test-case is going to need quite large values, because number * val(num) needs to overflow std::intmax_t. I tried extending the test-cases provided here to include std::intmax_t, but the challenge is how to programmatically determine the correct expected value, because the precision of double may not be good enough for that.

This fact highlights that the problem itself is absolutely non-trivial. It's not even clear what we should expect from these conversions. IMHO, the principle of least-surprise suggests the following for our value_cast, .force_in and friends:

  1. The conversion should be valid and correct for all values that are (approximately) representable in both the input and output type.
  2. The conversion should be approximately accurate to a resolution comparable with the larger of the source and target type.

I believe the current implementation is fairly close to these ideals, but likely breaks both for large std::intmax_t values, 1) when the conversion involves large rational factors, and 2) when it involves any irrational factors (due to the use of double).

@mpusz
Copy link
Owner

mpusz commented Jun 3, 2024

Sure, if you find a problematic case and have an idea on how to address it without breaking the currently working ones, then feel most welcome to provide a patch 😉

Thanks for adding more test cases! We need good testing here.

Copy link
Owner

@mpusz mpusz left a comment

Choose a reason for hiding this comment

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

Thanks!

@mpusz mpusz merged commit 60a8605 into mpusz:master Jun 3, 2024
167 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants