Skip to content

Conversation

@Arzaghi
Copy link
Contributor

@Arzaghi Arzaghi commented Aug 24, 2020

Fixes #1174

@Arzaghi Arzaghi requested a review from a team as a code owner August 24, 2020 14:54
@cbezault cbezault added the bug Something isn't working label Aug 24, 2020
Copy link
Contributor

@MattStephanson MattStephanson left a comment

Choose a reason for hiding this comment

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

  • For fisher_f_distribution, should we also reject here if _Px == _Ty{1}? That would generate infinity rather than NaN.

    STL/stl/inc/random

    Lines 4016 to 4019 in ba6f895

    _Beta_distribution<_Ty> _Dist(_Vx1, _Vx2);
    _Px = _Dist(_Eng);
    return (_Vx2 / _Vx1) * (_Px / (_Ty{1} - _Px));

  • The issue mentions student_t_distribution, but there's no change to it. It's not obvious to me how it would cause NaN, but _Rs == 0 would give infinity.

    STL/stl/inc/random

    Lines 4145 to 4156 in ba6f895

    for (;;) { // get a point inside unit circle
    _Vx1 = _Dist(_Eng);
    _Vx2 = _Dist(_Eng);
    _Rs = _Vx1 * _Vx1 + _Vx2 * _Vx2;
    if (_Rs < _Ty{1}) {
    break;
    }
    }
    _Rx0 = _CSTD sqrt(_Rs);
    return _Vx1 * _CSTD sqrt(_Par0._Nx * (_CSTD pow(_Rx0, -_Ty{4} / _Par0._Nx) - _Ty{1}) / _Rs);

@Arzaghi Arzaghi changed the title prevent random distributions return NaN output prevent random distributions return NaN/Inf output Aug 27, 2020
@Arzaghi
Copy link
Contributor Author

Arzaghi commented Aug 27, 2020

Here is my test program with a slight improvement to the @statementreply test code on the issue description.

#include <cassert>
#include <cmath>
#include <random>
#include <iostream>

#include "\tests\std\tests\GH_001017_discrete_distribution_out_of_range\bad_random_engine.hpp"

using namespace std;

template <class Distribution>
void test(Distribution&& distribution) {	
	for (bad_random_generator rng; !rng.has_cycled_through();) {
		const auto x = distribution(rng);
		assert(!isnan(x) && !isinf(x));
	}
}

int main() {
	test(normal_distribution<>{});
	std::cout << "normal_distribution\t\tok" << std::endl;

	test(lognormal_distribution<>{});	
	std::cout << "lognormal_distribution\t\tok" << std::endl;

	test(fisher_f_distribution<>{});	
	std::cout << "fisher_f_distribution\t\tok" << std::endl;

	test(student_t_distribution<>{});	
	std::cout << "student_t_distribution\t\tok" << std::endl;

	std::cout << "Done!";
	std::cin.get();
	return 0;
}

@Arzaghi Arzaghi force-pushed the Fix_Issue1174 branch 2 times, most recently from c28a9bb to 2d3dbe5 Compare September 2, 2020 02:58
Copy link
Contributor

@MattStephanson MattStephanson left a comment

Choose a reason for hiding this comment

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

Nice work, this looks mostly good to me. I just have a few minor points.

@Arzaghi Arzaghi changed the title prevent random distributions return NaN/Inf output prevent random distributions from returning NaN/Inf Sep 9, 2020
Copy link
Contributor

@cbezault cbezault left a comment

Choose a reason for hiding this comment

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

If there's something that I'm missing with regards to those static_casts then this LGTM. Otherwise could you just choose a consistent style for getting the type of variables you want?

Co-authored-by: Curtis J Bezault <[email protected]>
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.

This looks good to me, thanks. I have three superficial comments about comments (heh) and asserts, and one slightly more significant question about using a double versus _Ty constant. If the answer there is that comparing against 1.0e-4 is intentional and correct, I'm happy.

@StephanTLavavej StephanTLavavej removed their assignment Oct 1, 2020
@StephanTLavavej StephanTLavavej self-assigned this Oct 2, 2020
@StephanTLavavej StephanTLavavej merged commit a8b62af into microsoft:master Oct 3, 2020
@StephanTLavavej
Copy link
Member

Thanks again for fixing this silent bad codegen in <random> - the very worst category of bug! 🪲 ❌ => 😸 🎉

@Arzaghi Arzaghi deleted the Fix_Issue1174 branch October 3, 2020 14:22
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>: Some random number distributions could return NaN

4 participants