Skip to content

random: Fix γ-section implementation for Randomizer::getFloat() #12402

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

Merged
merged 2 commits into from
Oct 13, 2023

Conversation

TimWolla
Copy link
Member

The reference implementation of the "Drawing Random Floating-Point Numbers from an Interval" paper contains two mistakes that will result in erroneous values being returned under certain circumstances:

  • For large values of g the multiplication of k * g might overflow to infinity.
  • The value of ceilint() might exceed 2^53, possibly leading to a rounding error when promoting k to double within the multiplication of k * g.

This commit updates the implementation based on Prof. Goualard suggestions after reaching out to him. It will correctly handle inputs larger than 2^-1020 in absolute values. This limitation will be documented and those inputs possibly be rejected in a follow-up commit depending on performance concerns.

The reference implementation of the "Drawing Random Floating-Point Numbers from
an Interval" paper contains two mistakes that will result in erroneous values
being returned under certain circumstances:

- For large values of `g` the multiplication of `k * g` might overflow to
  infinity.
- The value of `ceilint()` might exceed 2^53, possibly leading to a rounding
  error when promoting `k` to double within the multiplication of `k * g`.

This commit updates the implementation based on Prof. Goualard suggestions
after reaching out to him. It will correctly handle inputs larger than 2^-1020
in absolute values. This limitation will be documented and those inputs
possibly be rejected in a follow-up commit depending on performance concerns.
Copy link
Member

@Girgias Girgias left a comment

Choose a reason for hiding this comment

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

This looks like black magic to me, but I trust that the implementation is sensible.

Copy link
Contributor

@zeriyoshi zeriyoshi left a comment

Choose a reason for hiding this comment

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

I have fully utilized my inadequate brain to check, and I think it's probably fine.
Although it would be better to obtain a review from a knowledgeable person in mathematics, Tim is probably already knowledgeable.

@TimWolla TimWolla merged commit 582b724 into php:PHP-8.3 Oct 13, 2023
@TimWolla TimWolla deleted the gammasection-overflow branch October 13, 2023 15:55
@TimWolla
Copy link
Member Author

The changes are actually pretty simple: Both issues are caused by k being too large, leading to floating point imprecision, thus the multiplication is effectively performed in two steps with smaller k:

max - k * g

is turned into:

4 * ((max / 4) - floor(k / 4) * g) - (k % 4) * g
= max - 4 * floor(k / 4) * g - (k % 4) * g
= max - (4 * floor(k / 4) + (k % 4)) * g
= max - k * g

This transformation is correct if max is at least 2^-1020, any smaller max / 4 rounds down to 0 and thus the reverse direction of multiplying with 4 doesn't give back the original value.

TimWolla added a commit to TimWolla/php-src that referenced this pull request Oct 13, 2023
* PHP-8.3:
  random: Fix γ-section implementation for Randomizer::getFloat() (php#12402)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants