<random>: Fix parameter initialization of piecewise_constant_distribution and piecewise_linear_distribution by statementreply · Pull Request #1032 · microsoft/STL

statementreply · 2020-07-11T09:11:42Z

Fixes #950 and fixes #1084.
Fixes AB#115939.

piecewise_linear_distribution

stl/inc/random

miscco · 2020-07-11T11:01:59Z

stl/inc/random

            if (2 <= _Ilist.size()) {
                _Bvec.assign(_Ilist);
+
+                for (size_t _Idx = 0; _Idx < _Bvec.size() - 1; ++_Idx) {


Do we need to reserve here?

There are several more occurrences in discrete_distribution, piecewise_constant_distribution and piecewise_linear_distribution where we could have reserved before populating vectors. Perhaps for another "performance" PR?

A separate performance PR would make sense. Cautionary note: a single reserve after vector construction is fine, but we should be careful to avoid reserve in anything that the user can call in a loop for a single vector, as that can trigger quadratic complexity. I'm specifically thinking about dist.param(const param_type&) here.

A pattern like this

STL/stl/inc/random

Lines 247 to 251 in 1fef5f7

void _Construct(_InIt _First, _InIt _Last) {

for (; _First != _Last; ++_First) {

_Myvec.push_back(static_cast<unsigned int>(*_First));

}

}

is ideally replaced with range-based algorithm, which would result in transformed iterators, and the vector would reserve automatically at least for random-access iterator.
Maybe just create issue for now and wait for ranges?

This needs to work in C++14 mode and thus will not have ranges avaialble.

miscco · 2020-07-11T11:04:49Z

stl/inc/random

            if (2 <= _Ilist.size()) {
                _Bvec.assign(_Ilist);
+
+                for (const auto& _Bval : _Bvec) {


At work I would ask for std::transform but here meh

And here, range-based transform would be too good for meh, since I expect it to be very short and clear, and enabling vector to auto-reserve

stl/inc/random

warning C4244: conversion from 'long double' to 'double'

statementreply · 2020-07-11T15:29:31Z

Could that be _Count==0 or even _Count = _STD min(_Count, 1u);

_STD max(_Count, size_t{1}) 😃

StephanTLavavej · 2020-07-12T00:44:13Z

Thanks - I verified that your AB#nnn citation properly linked the MS-internal bug to this PR. 🎉

statementreply · 2020-07-25T08:55:18Z

std/numerics/rand/rand.dis/rand.dist.samp/rand.dist.samp.pconst/eval.pass.cpp FAIL
std/numerics/rand/rand.dis/rand.dist.samp/rand.dist.samp.plinear/eval_param.pass.cpp FAIL
std/numerics/rand/rand.dis/rand.dist.samp/rand.dist.samp.plinear/eval.pass.cpp FAIL

These random number generation tests are failing because their pass criteria is too strict.

StephanTLavavej

Thanks! I think it's safe to say that you understand this code better than I do (and I've had a dozen years to learn). 😺 I've looked for inconsistencies, or anything that seems potentially ABI-concerning, and didn't find anything. (Also looked for sources of potential warnings, like float being used as a type, and that all looks good.) Adding test coverage to TR1 is a reasonable choice here, given how it already has a lot of the necessary infrastructure - we generally don't extend the legacy test suite but this merits an exception. libcxx providing test coverage (sometimes overly strict as you found) helps too.

StephanTLavavej · 2020-07-30T21:54:09Z

I've pushed a merge to resolve a conflict, accepting both changes to the libcxx skip lists (for random and lerp).

StephanTLavavej · 2020-08-03T22:04:15Z

I've pushed another merge, resolving #1119's STD changes in tr1.

CaseyCarter · 2020-08-12T02:08:55Z

tests/tr1/tests/random5/test.cpp


-    double arr[] = {1.0, 1.1, 1.2, 1.3, 1.4};
+    double arr[] = {1.0, 1.5, 2.0, 3.0, 4.0};
    dist_t dist3(STD initializer_list<double>(&arr[0], &arr[5]), myfn);


Ugh, using unrelated library internals in a test for no reason at all: tr1 things. (No change requested.)

Could be Good First Issue other than update define

A widespread problem #1184

StephanTLavavej · 2020-08-12T19:38:27Z

Thanks for fixing these bugs! 😸 This will ship in VS 2019 16.8 Preview 3.

statementreply added 2 commits July 11, 2020 16:57

Update tests for piecewise_constant_distribution and

4d895ae

piecewise_linear_distribution

Fix parameter initialization of piecewise_constant_distribution and

176efd4

piecewise_linear_distribution

statementreply requested a review from a team as a code owner July 11, 2020 09:11

clang-format

3708a87

statementreply changed the title ~~Fix parameter initialization of piecewise_constant_distribution and piecewise_linear_distribution~~ <random>: Fix parameter initialization of piecewise_constant_distribution and piecewise_linear_distribution Jul 11, 2020

statementreply marked this pull request as draft July 11, 2020 11:36

Fix incorrect boundary condition

f13643d

miscco reviewed Jul 11, 2020

View reviewed changes

statementreply added 2 commits July 11, 2020 21:39

Extract common computation

8627d9d

Update libcxx test result

2a07e1e

statementreply marked this pull request as ready for review July 11, 2020 13:45

AlexGuteniev reviewed Jul 11, 2020

View reviewed changes

stl/inc/random Outdated Show resolved Hide resolved

stl/inc/random Outdated Show resolved Hide resolved

statementreply added 2 commits July 11, 2020 23:04

Fix compile warning

0ea47b4

warning C4244: conversion from 'long double' to 'double'

Code review feedback: simplify vector assignment/initialization

1fef5f7

StephanTLavavej added the bug Something isn't working label Jul 12, 2020

This comment has been minimized.

Sign in to view

fsb4000 mentioned this pull request Jul 24, 2020

<random>: std::piecewise_linear_distribution incorrectly normalizes densities #1084

Closed

Update libcxx tests

45cb97b

StephanTLavavej self-assigned this Jul 25, 2020

StephanTLavavej approved these changes Jul 29, 2020

View reviewed changes

StephanTLavavej removed their assignment Jul 29, 2020

CaseyCarter self-assigned this Jul 29, 2020

Merge branch 'master' into fix_piecewise_linear_distribution

7055abe

Merge branch 'master' into fix_piecewise_linear_distribution

dca316a

This comment has been minimized.

Sign in to view

Merge branch 'master' into fix_piecewise_linear_distribution

29d8314

CaseyCarter approved these changes Aug 12, 2020

View reviewed changes

CaseyCarter removed their assignment Aug 12, 2020

StephanTLavavej self-assigned this Aug 12, 2020

StephanTLavavej merged commit 3224307 into microsoft:master Aug 12, 2020

statementreply deleted the fix_piecewise_linear_distribution branch April 17, 2021 11:03

	void _Construct(_InIt _First, _InIt _Last) {
	for (; _First != _Last; ++_First) {
	_Myvec.push_back(static_cast<unsigned int>(*_First));
	}
	}

Conversation

statementreply commented Jul 11, 2020 • edited by StephanTLavavej Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

statementreply commented Jul 11, 2020

Uh oh!

StephanTLavavej commented Jul 12, 2020

Uh oh!

This comment has been minimized.

statementreply commented Jul 25, 2020

Uh oh!

StephanTLavavej left a comment

Choose a reason for hiding this comment

Uh oh!

StephanTLavavej commented Jul 30, 2020

Uh oh!

StephanTLavavej commented Aug 3, 2020

Uh oh!

This comment has been minimized.

This comment has been minimized.

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

StephanTLavavej commented Aug 12, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

statementreply commented Jul 11, 2020 •

edited by StephanTLavavej

Loading