Skip to content

<random>: Fix parameter initialization of piecewise_constant_distribution and piecewise_linear_distribution #1032

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.

Sign up for GitHub

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
StephanTLavavej merged 12 commits into from
Aug 12, 2020
Merged

<random>: Fix parameter initialization of piecewise_constant_distribution and piecewise_linear_distribution #1032

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
12 commits
Select commit Hold shift + click to select a range
4d895ae
Update tests for piecewise_constant_distribution and
statementreply Jul 11, 2020
176efd4
Fix parameter initialization of piecewise_constant_distribution and
statementreply Jul 11, 2020
3708a87
clang-format
statementreply Jul 11, 2020
f13643d
Fix incorrect boundary condition
statementreply Jul 11, 2020
8627d9d
Extract common computation
statementreply Jul 11, 2020
2a07e1e
Update libcxx test result
statementreply Jul 11, 2020
0ea47b4
Fix compile warning
statementreply Jul 11, 2020
1fef5f7
Code review feedback: simplify vector assignment/initialization
statementreply Jul 11, 2020
45cb97b
Update libcxx tests
statementreply Jul 25, 2020
7055abe
Merge branch 'master' into fix_piecewise_linear_distribution
StephanTLavavej Jul 30, 2020
dca316a
Merge branch 'master' into fix_piecewise_linear_distribution
StephanTLavavej Aug 3, 2020
29d8314
Merge branch 'master' into fix_piecewise_linear_distribution
statementreply Aug 10, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
79 changes: 38 additions & 41 deletions stl/inc/random
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4528,17 +4528,16 @@ public:
struct param_type : _Mypbase { // parameter package
using distribution_type = piecewise_constant_distribution;

param_type() {
_Bvec.push_back(0);
_Bvec.push_back(1);
}
param_type() : _Bvec{0, 1} {}

template <class _InIt1, class _InIt2>
param_type(_InIt1 _First1, _InIt1 _Last1, _InIt2 _First2) : _Mypbase(_Noinit), _Bvec(_First1, _Last1) {
if (2 <= _Bvec.size()) {
for (size_t _Idx = 0; _Idx < _Bvec.size() - 1; ++_Idx) {
this->_Pvec.push_back(static_cast<double>(*_First2++));
}
} else { // default construct
_Bvec = {0, 1};
}

_Init();
Expand All @@ -4548,18 +4547,12 @@ public:
param_type(initializer_list<_Ty> _Ilist, _Fn _Func) : _Mypbase(_Noinit) {
if (2 <= _Ilist.size()) {
_Bvec.assign(_Ilist);
} else { // default construct
_Bvec.push_back(0);
_Bvec.push_back(1);
}

_Ty _Low = _Bvec.front();
_Ty _Range = _Bvec.back() - _Low;
size_t _Count = _Bvec.size() - 1;

_Range /= static_cast<_Ty>(_Count);
for (size_t _Idx = 0; _Idx < _Count; ++_Idx) {
_Pvec.push_back(_Func(_Low + _Idx * _Range));
for (size_t _Idx = 0; _Idx < _Bvec.size() - 1; ++_Idx) {
Copy link
Contributor

@miscco miscco Jul 11, 2020

Choose a reason for hiding this comment

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

Do we need to reserve here?

Copy link
Contributor Author

@statementreply statementreply Jul 11, 2020

Choose a reason for hiding this comment

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

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?

Copy link
Member

@StephanTLavavej StephanTLavavej Jul 12, 2020

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

@AlexGuteniev AlexGuteniev Jul 12, 2020

Choose a reason for hiding this comment

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

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?

Copy link
Member

@BillyONeal BillyONeal Jul 13, 2020

Choose a reason for hiding this comment

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

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

this->_Pvec.push_back(_Func(_Ty{0.5} * (_Bvec[_Idx] + _Bvec[_Idx + 1])));
}
} else { // default construct
_Bvec = {0, 1};
}

_Init();
Expand Down Expand Up @@ -4726,11 +4719,12 @@ public:
using result_type = _Ty;

struct param_type : _Mypbase { // parameter package
// TRANSITION, ABI: stores probability densities (N + 1 elements) in _Mybase::_Pvec
// this breaks invariants of discrete_distribution<size_t>::param_type
using distribution_type = piecewise_linear_distribution;

param_type() {
_Bvec.push_back(0);
_Bvec.push_back(1);
param_type() : _Bvec{0, 1} {
this->_Pvec.push_back(1.0);
}

template <class _InIt1, class _InIt2>
Expand All @@ -4739,6 +4733,8 @@ public:
for (size_t _Idx = 0; _Idx < _Bvec.size(); ++_Idx) {
this->_Pvec.push_back(static_cast<double>(*_First2++));
}
} else { // default construct
_Bvec = {0, 1};
}

_Init();
Expand All @@ -4748,18 +4744,12 @@ public:
param_type(initializer_list<_Ty> _Ilist, _Fn _Func) : _Mypbase(_Noinit) {
if (2 <= _Ilist.size()) {
_Bvec.assign(_Ilist);
} else { // default construct
_Bvec.push_back(0);
_Bvec.push_back(1);
}

_Ty _Low = _Bvec.front();
_Ty _Range = _Bvec.back() - _Low;
size_t _Count = _Bvec.size();

_Range /= static_cast<_Ty>(_Count);
for (size_t _Idx = 0; _Idx < _Count; ++_Idx) {
this->_Pvec.push_back(_Func(_Low + _Idx * _Range));
for (const auto& _Bval : _Bvec) {
Copy link
Contributor

@miscco miscco Jul 11, 2020

Choose a reason for hiding this comment

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

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

Copy link
Contributor

@AlexGuteniev AlexGuteniev Jul 12, 2020

Choose a reason for hiding this comment

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

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

this->_Pvec.push_back(_Func(_Bval));
}
} else { // default construct
_Bvec = {0, 1};
}

_Init();
Expand All @@ -4769,12 +4759,12 @@ public:
param_type(size_t _Count, _Ty _Low, _Ty _High, _Fn _Func) : _Mypbase(_Noinit) {
_Ty _Range = _High - _Low;
_STL_ASSERT(_Ty{0} < _Range, "invalid range for piecewise_linear_distribution");
if (_Count < 2) {
_Count = 2;
if (_Count < 1) {
_Count = 1;
}

_Range /= static_cast<double>(_Count);
for (size_t _Idx = 0; _Idx < _Count; ++_Idx) { // compute _Bvec and _Pvec
for (size_t _Idx = 0; _Idx <= _Count; ++_Idx) { // compute _Bvec and _Pvec
_Ty _Bval = _Low + _Idx * _Range;
_Bvec.push_back(_Bval);
this->_Pvec.push_back(_Func(_Bval));
Expand All @@ -4799,20 +4789,27 @@ public:
return _Ans;
}

_NODISCARD double _Piece_probability(const size_t _Idx) const {
return 0.5 * (this->_Pvec[_Idx] + this->_Pvec[_Idx + 1])
* static_cast<double>(_Bvec[_Idx + 1] - _Bvec[_Idx]);
}

void _Init(bool _Renorm = true) { // initialize
size_t _Size = this->_Pvec.size();
size_t _Idx;

if (_Renorm) {
if (this->_Pvec.empty()) {
this->_Pvec.push_back(1.0); // make empty vector degenerate
if (this->_Pvec.empty()) { // make empty vector degenerate
this->_Pvec = {1.0, 1.0};
} else { // normalize probabilities
double _Sum = 0;

_STL_ASSERT(0.0 <= this->_Pvec[0], "invalid probability for "
"piecewise_linear_distribution");
for (_Idx = 1; _Idx < _Size; ++_Idx) { // sum all probabilities
_STL_ASSERT(0.0 <= this->_Pvec[_Idx], "invalid probability for "
"piecewise_linear_distribution");
_Sum += 0.5 * (this->_Pvec[_Idx - 1] + this->_Pvec[_Idx]);
_Sum += _Piece_probability(_Idx - 1);
}

_STL_ASSERT(0.0 < _Sum, "invalid probability vector for "
Expand All @@ -4825,9 +4822,9 @@ public:
}
}

this->_Pcdf.assign(1, 0.5 * (this->_Pvec[0] + this->_Pvec[1]));
this->_Pcdf.assign(1, _Piece_probability(0));
for (_Idx = 2; _Idx < _Size; ++_Idx) {
this->_Pcdf.push_back(0.5 * (this->_Pvec[_Idx - 1] + this->_Pvec[_Idx]) + this->_Pcdf[_Idx - 2]);
this->_Pcdf.push_back(_Piece_probability(_Idx - 1) + this->_Pcdf[_Idx - 2]);
}
}

Expand Down Expand Up @@ -4894,14 +4891,14 @@ public:
_In(_Istr, _Val);
_Par0._Pvec.push_back(_Val);
}
_Par0._Init(false); // don't renormalize, just compute CDF

_Par._Bvec.clear();
for (size_t _Idx = _Par._Pvec.size(); 0 < _Idx; --_Idx) { // get a value and add to intervals vector
_Par0._Bvec.clear();
for (size_t _Idx = _Par0._Pvec.size(); 0 < _Idx; --_Idx) { // get a value and add to intervals vector
double _Val;
_In(_Istr, _Val);
_Par._Bvec.push_back(_Val);
_Par0._Bvec.push_back(_Val);
}
_Par0._Init(false); // don't renormalize, just compute CDF
return _Istr;
}

Expand Down
25 changes: 9 additions & 16 deletions tests/libcxx/expected_results.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -775,6 +775,15 @@ std/iterators/predef.iterators/insert.iterators/insert.iterator/types.pass.cpp F
std/numerics/complex.number/cmplx.over/conj.pass.cpp:0 FAIL
std/numerics/complex.number/cmplx.over/proj.pass.cpp:0 FAIL

# Assertion failed: std::abs(skew - x_skew) < 0.01
# Random number generation test with too strict pass criteria (test8 failure probability ~= 0.04)
std/numerics/rand/rand.dis/rand.dist.samp/rand.dist.samp.pconst/eval.pass.cpp FAIL

# Assertion failed: std::abs(f(u[i], a, m, bk, c) - double(i)/N) < .001
# Random number generation test with too strict pass criteria (test6 failure probability > 0.2)
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

# Assertion failed: (std::lerp(T(2.3), T(2.3), inf) == T(2.3))
# Asserts `(std::lerp(T(2.3), T(2.3), inf) == T(2.3))` and `std::isnan(std::lerp(T( 0), T( 0), inf))`
# They shouldn't behave differently. Both of them should probably return NaN.
Expand Down Expand Up @@ -817,22 +826,6 @@ std/numerics/rand/rand.dis/rand.dist.pois/rand.dist.pois.gamma/min.pass.cpp FAIL
std/numerics/rand/rand.dis/rand.dist.pois/rand.dist.pois.poisson/eval_param.pass.cpp FAIL
std/numerics/rand/rand.dis/rand.dist.pois/rand.dist.pois.poisson/eval.pass.cpp FAIL
std/numerics/rand/rand.dis/rand.dist.pois/rand.dist.pois.weibull/max.pass.cpp FAIL
std/numerics/rand/rand.dis/rand.dist.samp/rand.dist.samp.pconst/ctor_init_func.pass.cpp FAIL
std/numerics/rand/rand.dis/rand.dist.samp/rand.dist.samp.pconst/ctor_iterator.pass.cpp FAIL
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.pconst/param_ctor_init_func.pass.cpp FAIL
std/numerics/rand/rand.dis/rand.dist.samp/rand.dist.samp.pconst/param_ctor_iterator.pass.cpp FAIL
std/numerics/rand/rand.dis/rand.dist.samp/rand.dist.samp.plinear/ctor_default.pass.cpp FAIL
std/numerics/rand/rand.dis/rand.dist.samp/rand.dist.samp.plinear/ctor_func.pass.cpp FAIL
std/numerics/rand/rand.dis/rand.dist.samp/rand.dist.samp.plinear/ctor_init_func.pass.cpp FAIL
std/numerics/rand/rand.dis/rand.dist.samp/rand.dist.samp.plinear/ctor_iterator.pass.cpp FAIL
std/numerics/rand/rand.dis/rand.dist.samp/rand.dist.samp.plinear/ctor_param.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
std/numerics/rand/rand.dis/rand.dist.samp/rand.dist.samp.plinear/param_ctor_default.pass.cpp FAIL
std/numerics/rand/rand.dis/rand.dist.samp/rand.dist.samp.plinear/param_ctor_func.pass.cpp FAIL
std/numerics/rand/rand.dis/rand.dist.samp/rand.dist.samp.plinear/param_ctor_init_func.pass.cpp FAIL
std/numerics/rand/rand.dis/rand.dist.samp/rand.dist.samp.plinear/param_ctor_iterator.pass.cpp FAIL
std/numerics/rand/rand.dis/rand.dist.uni/rand.dist.uni.real/param_ctor.pass.cpp FAIL

# Not yet analyzed, likely STL bugs. Various assertions.
Expand Down
25 changes: 9 additions & 16 deletions tests/libcxx/skipped_tests.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -775,6 +775,15 @@ iterators\predef.iterators\insert.iterators\insert.iterator\types.pass.cpp
numerics\complex.number\cmplx.over\conj.pass.cpp
numerics\complex.number\cmplx.over\proj.pass.cpp

# Assertion failed: std::abs(skew - x_skew) < 0.01
# Random number generation test with too strict pass criteria (test8 failure probability ~= 0.04)
numerics\rand\rand.dis\rand.dist.samp\rand.dist.samp.pconst\eval.pass.cpp

# Assertion failed: std::abs(f(u[i], a, m, bk, c) - double(i)/N) < .001
# Random number generation test with too strict pass criteria (test6 failure probability > 0.2)
numerics\rand\rand.dis\rand.dist.samp\rand.dist.samp.plinear\eval_param.pass.cpp
numerics\rand\rand.dis\rand.dist.samp\rand.dist.samp.plinear\eval.pass.cpp

# Assertion failed: (std::lerp(T(2.3), T(2.3), inf) == T(2.3))
# Asserts `(std::lerp(T(2.3), T(2.3), inf) == T(2.3))` and `std::isnan(std::lerp(T( 0), T( 0), inf))`
# They shouldn't behave differently. Both of them should probably return NaN.
Expand Down Expand Up @@ -817,22 +826,6 @@ numerics\rand\rand.dis\rand.dist.pois\rand.dist.pois.gamma\min.pass.cpp
numerics\rand\rand.dis\rand.dist.pois\rand.dist.pois.poisson\eval_param.pass.cpp
numerics\rand\rand.dis\rand.dist.pois\rand.dist.pois.poisson\eval.pass.cpp
numerics\rand\rand.dis\rand.dist.pois\rand.dist.pois.weibull\max.pass.cpp
numerics\rand\rand.dis\rand.dist.samp\rand.dist.samp.pconst\ctor_init_func.pass.cpp
numerics\rand\rand.dis\rand.dist.samp\rand.dist.samp.pconst\ctor_iterator.pass.cpp
numerics\rand\rand.dis\rand.dist.samp\rand.dist.samp.pconst\eval.pass.cpp
numerics\rand\rand.dis\rand.dist.samp\rand.dist.samp.pconst\param_ctor_init_func.pass.cpp
numerics\rand\rand.dis\rand.dist.samp\rand.dist.samp.pconst\param_ctor_iterator.pass.cpp
numerics\rand\rand.dis\rand.dist.samp\rand.dist.samp.plinear\ctor_default.pass.cpp
numerics\rand\rand.dis\rand.dist.samp\rand.dist.samp.plinear\ctor_func.pass.cpp
numerics\rand\rand.dis\rand.dist.samp\rand.dist.samp.plinear\ctor_init_func.pass.cpp
numerics\rand\rand.dis\rand.dist.samp\rand.dist.samp.plinear\ctor_iterator.pass.cpp
numerics\rand\rand.dis\rand.dist.samp\rand.dist.samp.plinear\ctor_param.pass.cpp
numerics\rand\rand.dis\rand.dist.samp\rand.dist.samp.plinear\eval_param.pass.cpp
numerics\rand\rand.dis\rand.dist.samp\rand.dist.samp.plinear\eval.pass.cpp
numerics\rand\rand.dis\rand.dist.samp\rand.dist.samp.plinear\param_ctor_default.pass.cpp
numerics\rand\rand.dis\rand.dist.samp\rand.dist.samp.plinear\param_ctor_func.pass.cpp
numerics\rand\rand.dis\rand.dist.samp\rand.dist.samp.plinear\param_ctor_init_func.pass.cpp
numerics\rand\rand.dis\rand.dist.samp\rand.dist.samp.plinear\param_ctor_iterator.pass.cpp
numerics\rand\rand.dis\rand.dist.uni\rand.dist.uni.real\param_ctor.pass.cpp

# Not yet analyzed, likely STL bugs. Various assertions.
Expand Down
1 change: 1 addition & 0 deletions tests/tr1/include/tspec_random.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -135,4 +135,5 @@ void test_random() { // test all of the distributions
TESTR(student_t_distribution);
TESTR(discrete_distribution);
TESTR(piecewise_constant_distribution);
TESTR(piecewise_linear_distribution);
}
41 changes: 32 additions & 9 deletions tests/tr1/include/tspec_random_defs.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -309,13 +309,13 @@ One_arg weibull_distribution_vals[] = {
{FLIT(99.0), FLIT(0.0), FLIT(0.0)},
};

int _Random_test_fun(double x) {
return (int) x;
int _Random_test_fun_1(double x) {
return (int) x - 50;
}

typedef double piecewise_constant_distribution_type;
piecewise_constant_distribution<piecewise_constant_distribution_type> piecewise_constant_distribution_dist(
100, 50.0, 100.0, _Random_test_fun);
8, 50.0, 100.0, _Random_test_fun_1);
int piecewise_constant_distribution_smaller_ok = 0;
int piecewise_constant_distribution_larger_ok = 0;

Expand All @@ -325,15 +325,38 @@ One_arg piecewise_constant_distribution_vals[] = {
{FLIT(22.0), FLIT(0.0), FLIT(0.0)},
{FLIT(33.0), FLIT(0.0), FLIT(0.0)},
{FLIT(44.0), FLIT(0.0), FLIT(0.0)},
{FLIT(55.0), FLIT(0.014765100671140939452), FLIT(0.0)},
{FLIT(66.0), FLIT(0.017718120805369125953), FLIT(0.0)},
{FLIT(77.0), FLIT(0.02067114093959731419), FLIT(0.0)},
{FLIT(88.0), FLIT(0.023624161073825502428), FLIT(0.0)},
{FLIT(99.0), FLIT(0.026577181208053690665), FLIT(0.0)},
{FLIT(55.0), FLIT(0.0024489795918367346), FLIT(0.0)},
{FLIT(66.0), FLIT(0.012244897959183673), FLIT(0.0)},
{FLIT(77.0), FLIT(0.022857142857142857), FLIT(0.0)},
{FLIT(88.0), FLIT(0.0326530612244898), FLIT(0.0)},
{FLIT(99.0), FLIT(0.037551020408163265), FLIT(0.0)},
};

typedef double piecewise_linear_distribution_type;
piecewise_linear_distribution<piecewise_linear_distribution_type> piecewise_linear_distribution_dist(
8, 50.0, 100.0, _Random_test_fun_1);
int piecewise_linear_distribution_smaller_ok = 0;
int piecewise_linear_distribution_larger_ok = 0;

One_arg piecewise_linear_distribution_vals[] = {
{FLIT(0.0), FLIT(0.0), FLIT(0.0)},
{FLIT(11.0), FLIT(0.0), FLIT(0.0)},
{FLIT(22.0), FLIT(0.0), FLIT(0.0)},
{FLIT(33.0), FLIT(0.0), FLIT(0.0)},
{FLIT(44.0), FLIT(0.0), FLIT(0.0)},
{FLIT(55.0), FLIT(0.004467005076142132), FLIT(0.0)},
{FLIT(66.0), FLIT(0.013401015228426396), FLIT(0.0)},
{FLIT(77.0), FLIT(0.02233502538071066), FLIT(0.0)},
{FLIT(88.0), FLIT(0.03126903553299493), FLIT(0.0)},
{FLIT(99.0), FLIT(0.04020304568527919), FLIT(0.0)},
};

int _Random_test_fun_2(double x) {
return (int) x;
}

typedef unsigned int discrete_distribution_type;
discrete_distribution<discrete_distribution_type> discrete_distribution_dist(100, 70, 90, _Random_test_fun);
discrete_distribution<discrete_distribution_type> discrete_distribution_dist(100, 70, 90, _Random_test_fun_2);
int discrete_distribution_smaller_ok = 0;
int discrete_distribution_larger_ok = 0;

Expand Down
Loading