Fixes incorrect rounding in rejection method. #1159

MattStephanson · 2020-08-07T04:53:34Z

Fixes #1001.
The "0 <= _Res" part of the rejection test should be based on floor(x). With truncation, -1 < x <0 is mistakenly accepted, causing 0 to be overrepresented in the output.

Fixes #1123.
Adds function to calculate largest float that will truncate down to a given unsigned value.

- Should be based on floor(val) but implemented as trunction, so -1 < val < 0 is mistakenly accepted. The result 0 is therefore overrepresented.

statementreply · 2020-08-17T02:47:16Z

Could you please add tests for the bug fix to the test suite?

stl/inc/random

Most of the work is in a helper function that calculates the largest floating point number that truncates to less than or equal to a given unsigned integer.

stl/inc/random

statementreply · 2020-08-17T13:48:05Z

stl/inc/random

+        do {
+            _Val = _CSTD log(_NRAND(_Eng, _Ty1)) / _Par0._Log_1_p;
+        } while (_Val > _Ty1_max);


What would we like to do for possible values of the distribution that are outside the range of the integral result type?

Discard (reject): the probabilities of returning values within the range are all scaled by 1 / (1 - P(overflow)).

Saturate to numeric_limits<_Ty>::max(), and perhaps raise FE_INVALID: the probabilities of returning values less than numeric_limits<_Ty>::max() are unchanged, the probability of returning numeric_limits<_Ty>::max() is increased by P(overflow).

(no changes requested, maybe decision needed from maintainers)

My preference is (1). Raising an FP exception gives the most flexibility, but also biases one of the results for anyone who doesn't trap the exception (the typical case, I expect). Effectively truncating and rescaling the idealized distribution seems like the least surprising thing to do, but I don't feel particularly strong about it. FWIW, it looks like libcxx clamps while libstdc++ rejects.

https://github.com/llvm/llvm-project/blob/6a64079699e7b56badd292e39cad4b8bfe941aec/libcxx/include/random#L4719

https://github.com/gcc-mirror/gcc/blob/3eeede6de7f6021ad726f034401872f6d58b343d/libstdc%2B%2B-v3/include/bits/random.tcc#L1350

My preference is also 1.

stl/inc/random

tests/std/tests/GH_001001_random_rejection_rounding/test.cpp

- Need BSR operation for 64 bit types, even on x86. Borrowing _Bit_scan_reverse from charconv to xbit_ops.h to avoid duplication.

tests/std/tests/GH_001001_random_rejection_rounding/test.cpp

tests/std/tests/GH_001123_random_cast_out_of_range/test.cpp

stl/inc/random

tests/std/tests/GH_001123_random_cast_out_of_range/test.cpp

tests/std/tests/GH_001001_random_rejection_rounding/test.cpp

stl/inc/random

tests/std/tests/GH_001123_random_cast_out_of_range/test.cpp

stl/inc/random

StephanTLavavej

Thanks, this looks great. Upon re-review I found extremely minor stylistic issues; I'll validate that the proposed fixes build and pass your tests, and I'll go ahead and push a commit to save time.

stl/inc/random

tests/std/tests/GH_001123_random_cast_out_of_range/test.cpp

stl/inc/random

StephanTLavavej

FYI @cbezault, I pushed minor changes after you approved.

StephanTLavavej · 2020-10-02T23:52:17Z

I need to push a fix for /clr:pure. Manual targeted testing:

C:\Temp>type meow.cpp

#include <cassert>
#include <cstdio>
#include <limits>
#include <random>
using namespace std;

template <typename _Ty_32or64>
int TestMeow(_Ty_32or64 value) {
#ifdef _M_CEE_PURE
    constexpr auto _Ty_32or64_digits = numeric_limits<_Ty_32or64>::digits;
    return _Ty_32or64_digits - _Countl_zero_fallback(value);
#else // _M_CEE_PURE
    return _Bit_scan_reverse(value);
#endif // _M_CEE_PURE
}

int main() {
    assert(TestMeow(0x0000'0000u) == 0);

    assert(TestMeow(0x0000'0001u) == 1);
    assert(TestMeow(0x0000'0002u) == 2);
    assert(TestMeow(0x0000'0004u) == 3);
    assert(TestMeow(0x0000'0008u) == 4);

    assert(TestMeow(0x1000'0000u) == 29);
    assert(TestMeow(0x2000'0000u) == 30);
    assert(TestMeow(0x4000'0000u) == 31);
    assert(TestMeow(0x8000'0000u) == 32);

    assert(TestMeow(0x0000'0000'0000'0000ull) == 0);

    assert(TestMeow(0x0000'0000'0000'0001ull) == 1);
    assert(TestMeow(0x0000'0000'0000'0002ull) == 2);
    assert(TestMeow(0x0000'0000'0000'0004ull) == 3);
    assert(TestMeow(0x0000'0000'0000'0008ull) == 4);

    assert(TestMeow(0x1000'0000'0000'0000ull) == 61);
    assert(TestMeow(0x2000'0000'0000'0000ull) == 62);
    assert(TestMeow(0x4000'0000'0000'0000ull) == 63);
    assert(TestMeow(0x8000'0000'0000'0000ull) == 64);

    assert(TestMeow(0x0000'0003u) == 2);
    assert(TestMeow(0x0000'0005u) == 3);
    assert(TestMeow(0x0000'0009u) == 4);

    assert(TestMeow(0x10F1'234Au) == 29);
    assert(TestMeow(0x20F1'234Au) == 30);
    assert(TestMeow(0x40F1'234Au) == 31);
    assert(TestMeow(0x80F1'234Au) == 32);

    assert(TestMeow(0x0000'0000'0000'0003ull) == 2);
    assert(TestMeow(0x0000'0000'0000'0005ull) == 3);
    assert(TestMeow(0x0000'0000'0000'0009ull) == 4);

    assert(TestMeow(0x1000'000F'1234'A000ull) == 61);
    assert(TestMeow(0x2000'000F'1234'A000ull) == 62);
    assert(TestMeow(0x4000'000F'1234'A000ull) == 63);
    assert(TestMeow(0x8000'000F'1234'A000ull) == 64);

    puts("PASS");
}

C:\Temp>cl /EHsc /nologo /W4 meow.cpp && meow
meow.cpp
PASS

C:\Temp>cl /clr /nologo /W4 meow.cpp && meow
meow.cpp
PASS

C:\Temp>cl /clr:pure /nologo /W4 meow.cpp && meow
cl : Command line warning D9035 : option 'clr:pure' has been deprecated and will be removed in a future release
meow.cpp
S:\msvc\binaries\x86chk\inc\yvals.h(245): warning STL4001: /clr:pure is deprecated and will be REMOVED.
PASS

Most intrinsics, including _BitScanReverse, are unavailable in /clr:pure mode. The most targeted way to fix this is to call _Countl_zero_fallback which is available from <limits>. I've manually tested that these codepaths behave identically.

StephanTLavavej · 2020-10-03T02:14:37Z

Thanks again for fixing this silent bad codegen! We really appreciate it. 😺

Fixes incorrect rounding in rejection method.

63fdfc9

- Should be based on floor(val) but implemented as trunction, so -1 < val < 0 is mistakenly accepted. The result 0 is therefore overrepresented.

MattStephanson requested a review from a team as a code owner August 7, 2020 04:53

Fix formatting

4353d43

StephanTLavavej added the bug Something isn't working label Aug 8, 2020

mnatsuhara assigned cbezault Aug 12, 2020

statementreply reviewed Aug 17, 2020

View reviewed changes

stl/inc/random Outdated Show resolved Hide resolved

MattStephanson and others added 3 commits August 16, 2020 23:06

Fix out-of-range casts from double to integer types

58883ed

Most of the work is in a helper function that calculates the largest floating point number that truncates to less than or equal to a given unsigned integer.

Tests for microsoft#1001 and microsoft#1123

c575cb9

Merge branch 'master' into random_rejection_rounding

4999c1c

MattStephanson force-pushed the random_rejection_rounding branch from 5943374 to 1949495 Compare August 17, 2020 07:15

clang-format

d703774

MattStephanson force-pushed the random_rejection_rounding branch from 1949495 to d703774 Compare August 17, 2020 07:24

statementreply reviewed Aug 17, 2020

View reviewed changes

buildfix and code review comments

8e31f45

This comment has been minimized.

Sign in to view

MattStephanson and others added 3 commits August 18, 2020 16:42

buildfix - BSR for all integer widths

831f65f

- Need BSR operation for 64 bit types, even on x86. Borrowing _Bit_scan_reverse from charconv to xbit_ops.h to avoid duplication.

Merge branch 'master' into random_rejection_rounding

89c708e

Restore line endings from merge conflict

1d8d743

MattStephanson marked this pull request as draft August 19, 2020 00:18

This comment has been minimized.

Sign in to view

buildfix

b260904

MattStephanson marked this pull request as ready for review August 21, 2020 01:26

StephanTLavavej requested changes Aug 21, 2020

View reviewed changes

Apply suggestions from code review

f6d395a

statementreply reviewed Aug 21, 2020

View reviewed changes

stl/inc/random Outdated Show resolved Hide resolved

stl/inc/random Outdated Show resolved Hide resolved

suggestions from code review

3fa04cc

cbezault approved these changes Sep 2, 2020

View reviewed changes

mnatsuhara assigned StephanTLavavej Sep 2, 2020

mnatsuhara unassigned cbezault Sep 2, 2020

StephanTLavavej reviewed Oct 2, 2020

View reviewed changes

stl/inc/random Outdated Show resolved Hide resolved

tests/std/tests/GH_001123_random_cast_out_of_range/test.cpp Outdated Show resolved Hide resolved

stl/inc/random Outdated Show resolved Hide resolved

stl/inc/random Outdated Show resolved Hide resolved

Apply suggestions from code review

13a2527

StephanTLavavej approved these changes Oct 2, 2020

View reviewed changes

StephanTLavavej assigned StephanTLavavej and unassigned StephanTLavavej Oct 2, 2020

Fix /clr:pure compiler error.

4afbecc

Most intrinsics, including _BitScanReverse, are unavailable in /clr:pure mode. The most targeted way to fix this is to call _Countl_zero_fallback which is available from <limits>. I've manually tested that these codepaths behave identically.

StephanTLavavej approved these changes Oct 3, 2020

View reviewed changes

StephanTLavavej merged commit c385d02 into microsoft:master Oct 3, 2020

mnatsuhara mentioned this pull request Oct 13, 2020

<chrono> Partially implement P0355R7 #323

Merged

4 tasks

SuperWig mentioned this pull request Oct 21, 2020

tests: Consistently use unsigned int #1389

Closed

futuarmo mentioned this pull request Oct 21, 2020

Unsigned changed to unsigned int due to convention #1390

Merged

MattStephanson deleted the random_rejection_rounding branch January 1, 2021 07:42

Fixes incorrect rounding in rejection method. #1159

Fixes incorrect rounding in rejection method. #1159

Uh oh!

Conversation

MattStephanson commented Aug 7, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

statementreply commented Aug 17, 2020

Uh oh!

Uh oh!

Uh oh!

Uh oh!

statementreply Aug 17, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

MattStephanson Aug 22, 2020

Choose a reason for hiding this comment

Uh oh!

cbezault Sep 2, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

StephanTLavavej left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

StephanTLavavej left a comment

Choose a reason for hiding this comment

Uh oh!

StephanTLavavej commented Oct 2, 2020

Uh oh!

StephanTLavavej commented Oct 3, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

MattStephanson commented Aug 7, 2020 •

edited

Loading

statementreply Aug 17, 2020 •

edited

Loading

cbezault Sep 2, 2020 •

edited

Loading