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

<random>: Fix discrete_distribution result out of range #1025

Conversation

statementreply
Copy link
Contributor

Fix #1017.

Mathematically, _Par0._Pcdf.back() should be one. However, when it is actually slightly smaller than one due to rounding error, there is a small probability that _Px > _Par0._Pcdf.back(), and the original code returns the invalid value of _Par0._Pcdf.size().

Mathematically, _Par0._Pcdf.back() should be one. However, when it is
actually slightly smaller than one due to rounding error, there is a
small probability that _Px > _Par0._Pcdf.back() and the original code
returns the invalid value of _Par0._Pcdf.size().
@statementreply statementreply requested a review from a team as a code owner July 9, 2020 15:38
@statementreply
Copy link
Contributor Author

statementreply commented Jul 9, 2020

Question: do we add a (randomized?) test for low probability <random> bugs?

For this particular bug, the probability of returning an invalid result on each call is on the order of numeric_limits<double>::epsilon() * sqrt(N) for the average case and of numeric_limits<double>::epsilon() * N for the worst case, depending on distribution parameters and implementation details.

Copy link
Member

@CaseyCarter CaseyCarter left a comment

Choose a reason for hiding this comment

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

Related: Should we consider making _Pcdf an element shorter - leaving the final element as an implicit 1 - when we next break ABI?

stl/inc/random Outdated Show resolved Hide resolved
@CaseyCarter CaseyCarter added the bug Something isn't working label Jul 9, 2020
@CaseyCarter
Copy link
Member

CaseyCarter commented Jul 9, 2020

Question: do we add a (randomized?) test for low probability <random> bugs?

For this particular bug, the probability of returning an invalid result on each call is on the order of numeric_limits<double>::epsilon() * sqrt(N) for the average case and of numeric_limits<double>::epsilon() * N for the worst case, depending on distribution parameters and implementation details.

It would be nice to have at least a simple regression test with the repro from the issue. I wouldn't refuse a randomized test, but it seems like that test wouldn't provide much additional marginal coverage. (If you do choose to write a randomized test it needs to dump enough info to reproduce errors when found a la:

template <class T>
void assert_message_vector(const bool b, const char* const msg, const T seedValue) {
if (!b) {
cerr << msg << " failed for seed value: " << seedValue << "\n";
cerr << "This is a randomized test.\n";
cerr << "DO NOT IGNORE/RERUN THIS FAILURE.\n";
cerr << "You must report it to the STL maintainers.\n";
abort();
}
}

@statementreply statementreply marked this pull request as draft July 10, 2020 00:00
statementreply and others added 2 commits July 10, 2020 08:03
Use _Prev_iter

Co-authored-by: Casey Carter <cartec69@gmail.com>
@statementreply statementreply marked this pull request as ready for review July 10, 2020 14:51
@statementreply

This comment has been minimized.

Copy link
Member

@CaseyCarter CaseyCarter left a comment

Choose a reason for hiding this comment

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

Just a couple of style issues.

tests/std/include/bad_random_engine.hpp Outdated Show resolved Hide resolved
tests/std/include/bad_random_engine.hpp Outdated Show resolved Hide resolved
tests/std/include/bad_random_engine.hpp Outdated Show resolved Hide resolved
tests/std/include/bad_random_engine.hpp Outdated Show resolved Hide resolved
@CaseyCarter CaseyCarter removed their assignment Jul 22, 2020
Copy link
Member

@StephanTLavavej StephanTLavavej left a comment

Choose a reason for hiding this comment

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

Thanks! This looks great; I'll go ahead and push changes for a couple of conventions that we follow.

@CaseyCarter CaseyCarter self-assigned this Jul 27, 2020
@CaseyCarter CaseyCarter merged commit 8e8770c into microsoft:master Jul 27, 2020
@CaseyCarter
Copy link
Member

Thanks for the bugfix, @statementreply!

@CaseyCarter CaseyCarter removed their assignment Jul 27, 2020
CaseyCarter pushed a commit to CaseyCarter/STL that referenced this pull request Jul 28, 2020
Mathematically, `_Par0._Pcdf.back()` should be one. However, when it is
actually slightly smaller than one due to rounding error, there is a
small probability that `_Px > _Par0._Pcdf.back()` and the original code
returns the invalid value of `_Par0._Pcdf.size()`.
@statementreply statementreply deleted the fix_discrete_distribution_out_of_range branch August 10, 2020 13:51
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

<random>: discrete_distribution can return out-of-bound result
3 participants