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

change denominator of ofRandomuf from float to double. #3842

Merged
merged 6 commits into from Jun 3, 2015

Conversation

Projects
None yet
7 participants
@2bbb
Contributor

2bbb commented May 17, 2015

change from (RAND_MAX + 1.0f) to (static_cast<double>(RAND_MAX) + 1.0).

Because, if RAND_MAX is sufficiently large as larger than float precision, then RAND_MAX + 1.0f == RAND_MAX.
In other words, often rand() / (RAND_MAX + 1.0f) returns 1.0f

And, added static_cast<double> to clarify the meaning.

At the same time, replace same codes like rand() / (RAND_MAX + 1.0f) to ofRandomuf().
Maybe this tiny code will be inline expanded automatically when compiling.

more discussion about this: please see #3762

change denominator of ofRandomuf from float to double.
change from (RAND_MAX + 1.0f) to (RAND_MAX + 1.0).
because, if RAND_MAX is sufficiently large, then RAND_MAX + 1.0f == RAND_MAX.
in other words, often rand() / (RAND_MAX + 1.0f) returns 1.0f
see #3762
@kylemcdonald

This comment has been minimized.

Show comment
Hide comment
@kylemcdonald

kylemcdonald May 18, 2015

Contributor

this makes sense, i support the change.

another direction to go, if we start using c++11 in the core, would be http://www.cplusplus.com/reference/random/uniform_real_distribution/

Contributor

kylemcdonald commented May 18, 2015

this makes sense, i support the change.

another direction to go, if we start using c++11 in the core, would be http://www.cplusplus.com/reference/random/uniform_real_distribution/

@2bbb

This comment has been minimized.

Show comment
Hide comment
@2bbb

2bbb May 19, 2015

Contributor

hi @kylemcdonald
thanks for good comments.

I approve to use uniform_real_distribution if we can use C++11.
so, I had forgot that we can start to use C++11 in oF0.9.0 until now...

we can and we must develop more good ofRandom!!

Contributor

2bbb commented May 19, 2015

hi @kylemcdonald
thanks for good comments.

I approve to use uniform_real_distribution if we can use C++11.
so, I had forgot that we can start to use C++11 in oF0.9.0 until now...

we can and we must develop more good ofRandom!!

@danoli3

This comment has been minimized.

Show comment
Hide comment
@danoli3

danoli3 May 19, 2015

Member

We should add in PCG Random Number Generator. Includes everything we would ever want.
http://www.pcg-random.org (Also C++11 )

https://github.com/imneme/pcg-cpp

Member

danoli3 commented May 19, 2015

We should add in PCG Random Number Generator. Includes everything we would ever want.
http://www.pcg-random.org (Also C++11 )

https://github.com/imneme/pcg-cpp

@2bbb

This comment has been minimized.

Show comment
Hide comment
@2bbb

2bbb May 19, 2015

Contributor

i think this problem needs to fix as soon as possible , even if it is first aid.

because, there are description in header about ofRandom as

/// A random number in the range [0, max) will be returned.

and, we can write (althoug i think this is extreme example)
10.0 / (10 - static_cast<int>(ofRandom(0, 10)))
this code should be correct.
but, now, (It might be nervous, ) division by zero error occurs infrequently.

so, this happening makes sad people...

do you think what is correct root to beautiful world?

Contributor

2bbb commented May 19, 2015

i think this problem needs to fix as soon as possible , even if it is first aid.

because, there are description in header about ofRandom as

/// A random number in the range [0, max) will be returned.

and, we can write (althoug i think this is extreme example)
10.0 / (10 - static_cast<int>(ofRandom(0, 10)))
this code should be correct.
but, now, (It might be nervous, ) division by zero error occurs infrequently.

so, this happening makes sad people...

do you think what is correct root to beautiful world?

@kylemcdonald

This comment has been minimized.

Show comment
Hide comment
@kylemcdonald

kylemcdonald May 19, 2015

Contributor

i understand better now. i support this commit as a solution to rare divide by zero errors that could occur on certain platforms.

also, @2bbb do you know on which platforms RAND_MAX == RAND_MAX + 1.0f?

Contributor

kylemcdonald commented May 19, 2015

i understand better now. i support this commit as a solution to rare divide by zero errors that could occur on certain platforms.

also, @2bbb do you know on which platforms RAND_MAX == RAND_MAX + 1.0f?

@2bbb

This comment has been minimized.

Show comment
Hide comment
@2bbb

2bbb May 19, 2015

Contributor

@kylemcdonald
sorry, I know only

  • RAND_MAX == 0x7fffffff (i.e. RAND_MAX == RAND_MAX + 1.0f) in newest OSX10.10.3/Xcode6.3.1/clang, OSX10.8.5/Xcode5.0.2/clang, x86_64CentOS 6.5/gcc4.4.7
  • RAND_MAX == 0x7fff (i.e. RAND_MAX != RAND_MAX + 1.0f) in Visual Studio

or, just an idea, use PPM (or TMP) like this?

#if 0x1000000 <= RAND_MAX
// 0x1000000 == 16777216 is minimum value doesn't keep precision in IEEE 754 float single precision
// i.e. 0x1000000 + 1.0f == 0x1000000
#define denom 1.0
#else
#define denom 1.0f
#endif
Contributor

2bbb commented May 19, 2015

@kylemcdonald
sorry, I know only

  • RAND_MAX == 0x7fffffff (i.e. RAND_MAX == RAND_MAX + 1.0f) in newest OSX10.10.3/Xcode6.3.1/clang, OSX10.8.5/Xcode5.0.2/clang, x86_64CentOS 6.5/gcc4.4.7
  • RAND_MAX == 0x7fff (i.e. RAND_MAX != RAND_MAX + 1.0f) in Visual Studio

or, just an idea, use PPM (or TMP) like this?

#if 0x1000000 <= RAND_MAX
// 0x1000000 == 16777216 is minimum value doesn't keep precision in IEEE 754 float single precision
// i.e. 0x1000000 + 1.0f == 0x1000000
#define denom 1.0
#else
#define denom 1.0f
#endif
@kylemcdonald

This comment has been minimized.

Show comment
Hide comment
@kylemcdonald

kylemcdonald May 20, 2015

Contributor

i tried testing this branch, but it doesn't fix the bug for me.

#include "ofMain.h"

class ofApp : public ofBaseApp {
public:
    int n = 0;
    void setup() {
    }
    void update() {
        for(int i = 0; i < 100000000; i++) {
            float x = ofRandomuf();
            if(x == 1.0f) {
                ofLog() << "ofRandom() error after " << n;
                n = 0;
            } else {
                n++;
            }
        }
    }
    void draw() {
    }
};
int main() {
    ofSetupOpenGL(1280, 720, OF_WINDOW);
    ofRunApp(new ofApp());
}

the result should be that x != 1.0f but instead i get:

[notice ] ofRandom() error after 82364091
[notice ] ofRandom() error after 15067215
[notice ] ofRandom() error after 41564199
[notice ] ofRandom() error after 43254192
[notice ] ofRandom() error after 22500608
...

so the error has less to do with the addition, and more to do with the accuracy of the division.

also, i think it might be important for the other methods to be implemented the way they are, not by calling ofRandomuf(), because if they just scaled the results of ofRandomuf() then they have poor precision.

here is another definition that gives the correct behavior:

//--------------------------------------------------
float ofRandom(float max) {
    return ofRandom(0, max);
}

//--------------------------------------------------
float ofRandom(float x, float y) {
    // if there is no range, return the value
    if (x == y) return x;
    float high = MAX(x,y);
    float low = MIN(x,y);
    float range = high - low;
    float denominator = RAND_MAX / range;
    float result = rand() / denominator;
    result -= low;
    if(result == high) {
        return ofRandom(x, y);
    } else {
        return result;
    }
}

//--------------------------------------------------
float ofRandomf() {
    return ofRandom(-1, +1);
}

//--------------------------------------------------
float ofRandomuf() {
    return ofRandom(+1);
}

the chance of it executing twice is very small, and the distribution is still uniform.

let me know what you think.

Contributor

kylemcdonald commented May 20, 2015

i tried testing this branch, but it doesn't fix the bug for me.

#include "ofMain.h"

class ofApp : public ofBaseApp {
public:
    int n = 0;
    void setup() {
    }
    void update() {
        for(int i = 0; i < 100000000; i++) {
            float x = ofRandomuf();
            if(x == 1.0f) {
                ofLog() << "ofRandom() error after " << n;
                n = 0;
            } else {
                n++;
            }
        }
    }
    void draw() {
    }
};
int main() {
    ofSetupOpenGL(1280, 720, OF_WINDOW);
    ofRunApp(new ofApp());
}

the result should be that x != 1.0f but instead i get:

[notice ] ofRandom() error after 82364091
[notice ] ofRandom() error after 15067215
[notice ] ofRandom() error after 41564199
[notice ] ofRandom() error after 43254192
[notice ] ofRandom() error after 22500608
...

so the error has less to do with the addition, and more to do with the accuracy of the division.

also, i think it might be important for the other methods to be implemented the way they are, not by calling ofRandomuf(), because if they just scaled the results of ofRandomuf() then they have poor precision.

here is another definition that gives the correct behavior:

//--------------------------------------------------
float ofRandom(float max) {
    return ofRandom(0, max);
}

//--------------------------------------------------
float ofRandom(float x, float y) {
    // if there is no range, return the value
    if (x == y) return x;
    float high = MAX(x,y);
    float low = MIN(x,y);
    float range = high - low;
    float denominator = RAND_MAX / range;
    float result = rand() / denominator;
    result -= low;
    if(result == high) {
        return ofRandom(x, y);
    } else {
        return result;
    }
}

//--------------------------------------------------
float ofRandomf() {
    return ofRandom(-1, +1);
}

//--------------------------------------------------
float ofRandomuf() {
    return ofRandom(+1);
}

the chance of it executing twice is very small, and the distribution is still uniform.

let me know what you think.

@2bbb

This comment has been minimized.

Show comment
Hide comment
@2bbb

2bbb May 20, 2015

Contributor

oh my god...
i noticed...

i tested like this:

int main(int argc, char *argv[]) {
    for(unsigned long i = 0; i <= RAND_MAX; i++) {
        if(i / (RAND_MAX + 1.0) == 1.0) { std::cout << i << std::endl; exit(-1); }
        if(i == RAND_MAX) std::cout << "correct" << std::endl;
    }
}

execute this, and i got correct
but, this is not casted to float... correct test is this:

int main(int argc, char *argv[]) {
    for(unsigned long i = 0; i <= RAND_MAX; i++) {
        if(static_cast<float>(i / (RAND_MAX + 1.0)) == 1.0f) { std::cout << i << std::endl; exit(-1); }
        if(i == RAND_MAX) std::cout << "correct" << std::endl;
    }
}

and now, i got 2147483584...

so, new code is written by kyle is arguably correct. because, this code checks if(result == high).
i think this solution is not beatiful. but, sadly, i don't have other good idea now.

i will try thinking about other elegant solution a little more.
however, what we using kyle's solution now, and we doing discuss about new random function with uniform_real_distribution or PCG or other C++11 generation technic might be more constructive. i think.

Contributor

2bbb commented May 20, 2015

oh my god...
i noticed...

i tested like this:

int main(int argc, char *argv[]) {
    for(unsigned long i = 0; i <= RAND_MAX; i++) {
        if(i / (RAND_MAX + 1.0) == 1.0) { std::cout << i << std::endl; exit(-1); }
        if(i == RAND_MAX) std::cout << "correct" << std::endl;
    }
}

execute this, and i got correct
but, this is not casted to float... correct test is this:

int main(int argc, char *argv[]) {
    for(unsigned long i = 0; i <= RAND_MAX; i++) {
        if(static_cast<float>(i / (RAND_MAX + 1.0)) == 1.0f) { std::cout << i << std::endl; exit(-1); }
        if(i == RAND_MAX) std::cout << "correct" << std::endl;
    }
}

and now, i got 2147483584...

so, new code is written by kyle is arguably correct. because, this code checks if(result == high).
i think this solution is not beatiful. but, sadly, i don't have other good idea now.

i will try thinking about other elegant solution a little more.
however, what we using kyle's solution now, and we doing discuss about new random function with uniform_real_distribution or PCG or other C++11 generation technic might be more constructive. i think.

@arturoc

This comment has been minimized.

Show comment
Hide comment
@arturoc

arturoc May 20, 2015

Member

this is really a precision problem with floats but when you put your original solution in the ofRandomuf function you are still returning a float and in kyle's test still comparing with a float.

changing ofRandomuf to:

double ofRandomuf() {
    return rand() / double(RAND_MAX + 1.0);
}

and the test to:

    void update() {
        for(int i = 0; i < 100000000; i++) {
            double x = ofRandomuf();
            if(x == 1.0) {
                ofLog() << "ofRandom() error after " << n;
                n = 0;
            } else {
                n++;
            }
        }
    }

solves it for me.

Member

arturoc commented May 20, 2015

this is really a precision problem with floats but when you put your original solution in the ofRandomuf function you are still returning a float and in kyle's test still comparing with a float.

changing ofRandomuf to:

double ofRandomuf() {
    return rand() / double(RAND_MAX + 1.0);
}

and the test to:

    void update() {
        for(int i = 0; i < 100000000; i++) {
            double x = ofRandomuf();
            if(x == 1.0) {
                ofLog() << "ofRandom() error after " << n;
                n = 0;
            } else {
                n++;
            }
        }
    }

solves it for me.

@2bbb

This comment has been minimized.

Show comment
Hide comment
@2bbb

2bbb May 20, 2015

Contributor

@arturoc

yeah, thanks comments.
i know changing return type from float to double is right way.
but, could we change?

i think many of the oF code is use float.
and, if we receive the return value into double then it is clear. but, doesn't same problem happen again if receive into float?

i think this solution is perfect answer. but i think this solution will contain a problem sometime, only on oF culture.
i agree to kyle's solution.

Contributor

2bbb commented May 20, 2015

@arturoc

yeah, thanks comments.
i know changing return type from float to double is right way.
but, could we change?

i think many of the oF code is use float.
and, if we receive the return value into double then it is clear. but, doesn't same problem happen again if receive into float?

i think this solution is perfect answer. but i think this solution will contain a problem sometime, only on oF culture.
i agree to kyle's solution.

@arturoc

This comment has been minimized.

Show comment
Hide comment
@arturoc

arturoc May 20, 2015

Member

yeah, that's correct.

this also solves it and avoids the rescursive call and the conditional:

return rand() / float(RAND_MAX) * (1.0f-std::numeric_limits<float>::epsilon())
Member

arturoc commented May 20, 2015

yeah, that's correct.

this also solves it and avoids the rescursive call and the conditional:

return rand() / float(RAND_MAX) * (1.0f-std::numeric_limits<float>::epsilon())
@kylemcdonald

This comment has been minimized.

Show comment
Hide comment
@kylemcdonald

kylemcdonald May 20, 2015

Contributor

@arturoc the double solution is problematic because when old code is run that stores the value in a float it could still be equal to 1.0 sometimes:

        double asDouble = RAND_MAX / double(RAND_MAX + 1.0);
        ofLog() << (asDouble == 1.0);
        ofLog() << (asDouble == 1.0f);
        float asSingle = asDouble;
        ofLog() << (asSingle == 1.0);
        ofLog() << (asSingle == 1.0f);
[notice ] 0
[notice ] 0
[notice ] 1
[notice ] 1

the epsilon solution works, but i still propose we implement it in ofRandom(x, y) and call that function from the others.

//--------------------------------------------------
float ofRandom(float max) {
    return ofRandom(0, max);
}

//--------------------------------------------------
float ofRandom(float x, float y) {
    // if there is no range, return the value
    if (x == y) return x;
    float high = MAX(x,y);
    float low = MIN(x,y);
    float range = high - low;
    range -= std::numeric_limits<float>::epsilon();
    float denominator = RAND_MAX / range;
    float result = rand() / denominator;
    result -= low;
    return result;
}

//--------------------------------------------------
float ofRandomf() {
    return ofRandom(-1, +1);
}

//--------------------------------------------------
float ofRandomuf() {
    return ofRandom(+1);
}
Contributor

kylemcdonald commented May 20, 2015

@arturoc the double solution is problematic because when old code is run that stores the value in a float it could still be equal to 1.0 sometimes:

        double asDouble = RAND_MAX / double(RAND_MAX + 1.0);
        ofLog() << (asDouble == 1.0);
        ofLog() << (asDouble == 1.0f);
        float asSingle = asDouble;
        ofLog() << (asSingle == 1.0);
        ofLog() << (asSingle == 1.0f);
[notice ] 0
[notice ] 0
[notice ] 1
[notice ] 1

the epsilon solution works, but i still propose we implement it in ofRandom(x, y) and call that function from the others.

//--------------------------------------------------
float ofRandom(float max) {
    return ofRandom(0, max);
}

//--------------------------------------------------
float ofRandom(float x, float y) {
    // if there is no range, return the value
    if (x == y) return x;
    float high = MAX(x,y);
    float low = MIN(x,y);
    float range = high - low;
    range -= std::numeric_limits<float>::epsilon();
    float denominator = RAND_MAX / range;
    float result = rand() / denominator;
    result -= low;
    return result;
}

//--------------------------------------------------
float ofRandomf() {
    return ofRandom(-1, +1);
}

//--------------------------------------------------
float ofRandomuf() {
    return ofRandom(+1);
}
@arturoc

This comment has been minimized.

Show comment
Hide comment
@arturoc

arturoc May 20, 2015

Member

yes, seems easier to maintain. the only potential problem i see is that MAX and MIN are doing 2 conditionals each which could introduce some overhead but we should do some performance tests to check it.

also shouldn't result -= low; be result += low;

Member

arturoc commented May 20, 2015

yes, seems easier to maintain. the only potential problem i see is that MAX and MIN are doing 2 conditionals each which could introduce some overhead but we should do some performance tests to check it.

also shouldn't result -= low; be result += low;

@kylemcdonald

This comment has been minimized.

Show comment
Hide comment
@kylemcdonald

kylemcdonald May 21, 2015

Contributor

hah, good catch :)

@2bbb want to prepare a new PR considering all these things?

Contributor

kylemcdonald commented May 21, 2015

hah, good catch :)

@2bbb want to prepare a new PR considering all these things?

@2bbb

This comment has been minimized.

Show comment
Hide comment
@2bbb

2bbb May 21, 2015

Contributor

@kylemcdonald @arturoc
i feel you are great!
and i pushed source is based on kyle's code with artruro pointed out.

i do test that gives some value range for ofRandom, and all is correct.
but, i found a tiny concern at this small code.

float large_range_check(float low, float high) {
    float range = high - low;
    float little_bit_small_range = (high - low) - std::numeric_limits<float>::epsilon();

    printf("%x\n", *(reinterpret_cast<int *>(&range)));
    printf("%x\n", *(reinterpret_cast<int *>(&little_bit_small_range)));
}

large_range_check(0.0f, std::numeric_limits<float>::max());
large_range_check(0.0f, 3.0f);

and results:

7f7fffff
7f7fffff
40400000
40400000

i'm thinking about "really does this fact has nothing like problems?".

Contributor

2bbb commented May 21, 2015

@kylemcdonald @arturoc
i feel you are great!
and i pushed source is based on kyle's code with artruro pointed out.

i do test that gives some value range for ofRandom, and all is correct.
but, i found a tiny concern at this small code.

float large_range_check(float low, float high) {
    float range = high - low;
    float little_bit_small_range = (high - low) - std::numeric_limits<float>::epsilon();

    printf("%x\n", *(reinterpret_cast<int *>(&range)));
    printf("%x\n", *(reinterpret_cast<int *>(&little_bit_small_range)));
}

large_range_check(0.0f, std::numeric_limits<float>::max());
large_range_check(0.0f, 3.0f);

and results:

7f7fffff
7f7fffff
40400000
40400000

i'm thinking about "really does this fact has nothing like problems?".

@2bbb

This comment has been minimized.

Show comment
Hide comment
@2bbb

2bbb May 21, 2015

Contributor

hi.

i thought another way to make range little bit small like this:

    float range = high - low;
    range *= (1.0f - std::numeric_limits<float>::epsilon());

and test:

float large_range_check(float low, float high) {
    float range = high - low;
    float little_bit_small_range = (high - low) * (1.0f - std::numeric_limits<float>::epsilon());

    printf("%x\n", *(reinterpret_cast<int *>(&range)));
    printf("%x\n", *(reinterpret_cast<int *>(&little_bit_small_range)));
}

large_range_check(0.0f, std::numeric_limits<float>::max());
large_range_check(0.0f, 3.0f);

results:

and results:

7f7fffff
7f7ffffd
40e00000
40dffffe

but i don't know we really need or not this difference to solve this problem.
and, this way will pay cost higher than kyle's way a little.

I want to know you guys opinion.

Contributor

2bbb commented May 21, 2015

hi.

i thought another way to make range little bit small like this:

    float range = high - low;
    range *= (1.0f - std::numeric_limits<float>::epsilon());

and test:

float large_range_check(float low, float high) {
    float range = high - low;
    float little_bit_small_range = (high - low) * (1.0f - std::numeric_limits<float>::epsilon());

    printf("%x\n", *(reinterpret_cast<int *>(&range)));
    printf("%x\n", *(reinterpret_cast<int *>(&little_bit_small_range)));
}

large_range_check(0.0f, std::numeric_limits<float>::max());
large_range_check(0.0f, 3.0f);

results:

and results:

7f7fffff
7f7ffffd
40e00000
40dffffe

but i don't know we really need or not this difference to solve this problem.
and, this way will pay cost higher than kyle's way a little.

I want to know you guys opinion.

@kylemcdonald

This comment has been minimized.

Show comment
Hide comment
@kylemcdonald

kylemcdonald May 22, 2015

Contributor

i think i found the correct solution!

float large_range_check(float low, float high) {
    float range = high - low;
    float little_bit_small_range = nexttowardf(range, 0);

    printf("%x\n", *(reinterpret_cast<int *>(&range)));
    printf("%x\n", *(reinterpret_cast<int *>(&little_bit_small_range)));
}

http://en.cppreference.com/w/c/numeric/math/nextafter

Contributor

kylemcdonald commented May 22, 2015

i think i found the correct solution!

float large_range_check(float low, float high) {
    float range = high - low;
    float little_bit_small_range = nexttowardf(range, 0);

    printf("%x\n", *(reinterpret_cast<int *>(&range)));
    printf("%x\n", *(reinterpret_cast<int *>(&little_bit_small_range)));
}

http://en.cppreference.com/w/c/numeric/math/nextafter

@2bbb

This comment has been minimized.

Show comment
Hide comment
@2bbb

2bbb May 22, 2015

Contributor

wow!!! so cool!!!
i didn't know this function!!

i believe this is reliable best solution! (and i believe this is optimized!!) 💯

i think i will try to update PR with this function. is it good?

Contributor

2bbb commented May 22, 2015

wow!!! so cool!!!
i didn't know this function!!

i believe this is reliable best solution! (and i believe this is optimized!!) 💯

i think i will try to update PR with this function. is it good?

@2bbb

This comment has been minimized.

Show comment
Hide comment
@2bbb

2bbb May 22, 2015

Contributor

i did check correctness and cost with this code:

#include "ofMain.h"

class testRandomApp : public ofBaseApp {
    size_t errorCount;
public:
    void setup() {
        errorCount = 0;
        ofSetFrameRate(0.0f);
    }

    void update(){
        float p;
        float from = -(ofGetFrameNum() + 1) * 100, to = (ofGetFrameNum() + 1) * 100;
        for(int i = 0; i < 10000000; i++) {
            p = ofRandom(from, to);
            if(p < from || to <= p) {
                printf("omg: (%12.12f, %12.12f), %12.12f\n", from, to, p);
                errorCount++;
            }
        }
        ofSetWindowTitle(ofToString(ofGetFrameRate()));

        if(ofGetFrameNum() == 10000) {
            ofLogNotice() << "total error: " << errorCount << " / elpased: " << ofGetElapsedTimef();
            ofExit();
        }
    }
};

int main() {
    ofSetupOpenGL(200, 200, OF_WINDOW);
    ofRunApp(new testRandomApp());
}

implementation ofRandom :

float ofRandom(float x, float y) {
    // if there is no range, return the value
    if (x == y) return x;
    float high = MAX(x, y);
    float low = MIN(x, y);
    float range = high - low;
    range = make_small(range);
    float denominator = RAND_MAX / range;
    float result = rand() / denominator;
    result += low;
    return result;
}

implementation of make_small A:

    range = nexttowardf(range, 0.0f)

results of A:

omg: (-1800.000000000000, 1800.000000000000), 1800.000000000000
omg: (-6200.000000000000, 6200.000000000000), 6200.000000000000
omg: (-12400.000000000000, 12400.000000000000), 12400.000000000000
omg: (-16100.000000000000, 16100.000000000000), 16100.000000000000
omg: (-25900.000000000000, 25900.000000000000), 25900.000000000000
omg: (-28800.000000000000, 28800.000000000000), 28800.000000000000
omg: (-29800.000000000000, 29800.000000000000), 29800.000000000000
omg: (-30000.000000000000, 30000.000000000000), 30000.000000000000
...
omg: (-990200.000000000000, 990200.000000000000), 990200.000000000000
omg: (-993100.000000000000, 993100.000000000000), 993100.000000000000
omg: (-993400.000000000000, 993400.000000000000), 993400.000000000000
omg: (-993700.000000000000, 993700.000000000000), 993700.000000000000
omg: (-995200.000000000000, 995200.000000000000), 995200.000000000000
omg: (-995600.000000000000, 995600.000000000000), 995600.000000000000
omg: (-997100.000000000000, 997100.000000000000), 997100.000000000000
[notice ] total error: 265 / elpased: 1498.25

implementation of make_small B:

    range *= std::numeric_limits<float>::epsilon();

results of B:

omg: (-97900.000000000000, 97900.000000000000), 97900.000000000000
omg: (-187100.000000000000, 187100.000000000000), 187100.000000000000
omg: (-191300.000000000000, 191300.000000000000), 191300.000000000000
omg: (-374200.000000000000, 374200.000000000000), 374200.000000000000
omg: (-389300.000000000000, 389300.000000000000), 389300.000000000000
omg: (-391800.000000000000, 391800.000000000000), 391800.000000000000
omg: (-748400.000000000000, 748400.000000000000), 748400.000000000000
omg: (-773800.000000000000, 773800.000000000000), 773800.000000000000
omg: (-776400.000000000000, 776400.000000000000), 776400.000000000000
[notice ] total error: 9 / elpased: 891.372

maybe this is not a exacting test.
but i got a conjecture and sad fact.

  • A is not faster than B.
    • maybe A costs about 1.5times of B's cost.
  • each ways are NOT perfect when the big value is given.
    • and, (although i don't know why) i think A gave many incorrect results more than B with a significant difference. (perhaps this cause is NOT the random number.)

this problem is too difficult than i thought...
but, i feel exciting!
i wanna get the right way as much as possible.

i will be lost in thought about this again.

Contributor

2bbb commented May 22, 2015

i did check correctness and cost with this code:

#include "ofMain.h"

class testRandomApp : public ofBaseApp {
    size_t errorCount;
public:
    void setup() {
        errorCount = 0;
        ofSetFrameRate(0.0f);
    }

    void update(){
        float p;
        float from = -(ofGetFrameNum() + 1) * 100, to = (ofGetFrameNum() + 1) * 100;
        for(int i = 0; i < 10000000; i++) {
            p = ofRandom(from, to);
            if(p < from || to <= p) {
                printf("omg: (%12.12f, %12.12f), %12.12f\n", from, to, p);
                errorCount++;
            }
        }
        ofSetWindowTitle(ofToString(ofGetFrameRate()));

        if(ofGetFrameNum() == 10000) {
            ofLogNotice() << "total error: " << errorCount << " / elpased: " << ofGetElapsedTimef();
            ofExit();
        }
    }
};

int main() {
    ofSetupOpenGL(200, 200, OF_WINDOW);
    ofRunApp(new testRandomApp());
}

implementation ofRandom :

float ofRandom(float x, float y) {
    // if there is no range, return the value
    if (x == y) return x;
    float high = MAX(x, y);
    float low = MIN(x, y);
    float range = high - low;
    range = make_small(range);
    float denominator = RAND_MAX / range;
    float result = rand() / denominator;
    result += low;
    return result;
}

implementation of make_small A:

    range = nexttowardf(range, 0.0f)

results of A:

omg: (-1800.000000000000, 1800.000000000000), 1800.000000000000
omg: (-6200.000000000000, 6200.000000000000), 6200.000000000000
omg: (-12400.000000000000, 12400.000000000000), 12400.000000000000
omg: (-16100.000000000000, 16100.000000000000), 16100.000000000000
omg: (-25900.000000000000, 25900.000000000000), 25900.000000000000
omg: (-28800.000000000000, 28800.000000000000), 28800.000000000000
omg: (-29800.000000000000, 29800.000000000000), 29800.000000000000
omg: (-30000.000000000000, 30000.000000000000), 30000.000000000000
...
omg: (-990200.000000000000, 990200.000000000000), 990200.000000000000
omg: (-993100.000000000000, 993100.000000000000), 993100.000000000000
omg: (-993400.000000000000, 993400.000000000000), 993400.000000000000
omg: (-993700.000000000000, 993700.000000000000), 993700.000000000000
omg: (-995200.000000000000, 995200.000000000000), 995200.000000000000
omg: (-995600.000000000000, 995600.000000000000), 995600.000000000000
omg: (-997100.000000000000, 997100.000000000000), 997100.000000000000
[notice ] total error: 265 / elpased: 1498.25

implementation of make_small B:

    range *= std::numeric_limits<float>::epsilon();

results of B:

omg: (-97900.000000000000, 97900.000000000000), 97900.000000000000
omg: (-187100.000000000000, 187100.000000000000), 187100.000000000000
omg: (-191300.000000000000, 191300.000000000000), 191300.000000000000
omg: (-374200.000000000000, 374200.000000000000), 374200.000000000000
omg: (-389300.000000000000, 389300.000000000000), 389300.000000000000
omg: (-391800.000000000000, 391800.000000000000), 391800.000000000000
omg: (-748400.000000000000, 748400.000000000000), 748400.000000000000
omg: (-773800.000000000000, 773800.000000000000), 773800.000000000000
omg: (-776400.000000000000, 776400.000000000000), 776400.000000000000
[notice ] total error: 9 / elpased: 891.372

maybe this is not a exacting test.
but i got a conjecture and sad fact.

  • A is not faster than B.
    • maybe A costs about 1.5times of B's cost.
  • each ways are NOT perfect when the big value is given.
    • and, (although i don't know why) i think A gave many incorrect results more than B with a significant difference. (perhaps this cause is NOT the random number.)

this problem is too difficult than i thought...
but, i feel exciting!
i wanna get the right way as much as possible.

i will be lost in thought about this again.

@2bbb

This comment has been minimized.

Show comment
Hide comment
@2bbb

2bbb May 23, 2015

Contributor

i found simple way that might pass previous my test code in more wide range.
like this:

float ofRandom(float x, float y) {
    // if there is no range, return the value
    if (x == y) return x;
    float high = MAX(x, y);
    float low = MIN(x, y);
    float range = high - low;
    float denominator = RAND_MAX / range;
    float result = rand() / denominator;
    result *= 1.0f - numeric_limits<float>::epsilon();
    result += low;
    return result;
}

results of some tests:

omg: (-750500.000000000000, 750500.000000000000), 750500.000000000000
omg: (-762100.000000000000, 762100.000000000000), 762100.000000000000
omg: (-765300.000000000000, 765300.000000000000), 765300.000000000000
[notice ] total error: 3 / elpased: 992.009
omg: (-387100.000000000000, 387100.000000000000), 387100.000000000000
omg: (-754700.000000000000, 754700.000000000000), 754700.000000000000
omg: (-768700.000000000000, 768700.000000000000), 768700.000000000000
omg: (-777700.000000000000, 777700.000000000000), 777700.000000000000
omg: (-778200.000000000000, 778200.000000000000), 778200.000000000000
[notice ] total error: 5 / elpased: 962.492
omg: (-389100.000000000000, 389100.000000000000), 389100.000000000000
omg: (-763800.000000000000, 763800.000000000000), 763800.000000000000
omg: (-765300.000000000000, 765300.000000000000), 765300.000000000000
[notice ] total error: 3 / elpased: 921.356

i think about A in previous post, maybe nexttowardf is similar to x - numeric_limist<float>::epsilon() than x * (1.0f - numeric_limist<float>::epsilon()).
and, maybe addition's effect range is smaller than multiplication like B.

next, i think about B, maybe that works effectively than A. but that has many operation after the affecting than this way.
maybe, effect of smallize is become irrelevant by some operation.

so, epsilon can affects only too tiny range, it is easily erased.
and i tried to give effects at more behind.

i know this is not perfect. but i think this is more better way.
what are you think about this?

Contributor

2bbb commented May 23, 2015

i found simple way that might pass previous my test code in more wide range.
like this:

float ofRandom(float x, float y) {
    // if there is no range, return the value
    if (x == y) return x;
    float high = MAX(x, y);
    float low = MIN(x, y);
    float range = high - low;
    float denominator = RAND_MAX / range;
    float result = rand() / denominator;
    result *= 1.0f - numeric_limits<float>::epsilon();
    result += low;
    return result;
}

results of some tests:

omg: (-750500.000000000000, 750500.000000000000), 750500.000000000000
omg: (-762100.000000000000, 762100.000000000000), 762100.000000000000
omg: (-765300.000000000000, 765300.000000000000), 765300.000000000000
[notice ] total error: 3 / elpased: 992.009
omg: (-387100.000000000000, 387100.000000000000), 387100.000000000000
omg: (-754700.000000000000, 754700.000000000000), 754700.000000000000
omg: (-768700.000000000000, 768700.000000000000), 768700.000000000000
omg: (-777700.000000000000, 777700.000000000000), 777700.000000000000
omg: (-778200.000000000000, 778200.000000000000), 778200.000000000000
[notice ] total error: 5 / elpased: 962.492
omg: (-389100.000000000000, 389100.000000000000), 389100.000000000000
omg: (-763800.000000000000, 763800.000000000000), 763800.000000000000
omg: (-765300.000000000000, 765300.000000000000), 765300.000000000000
[notice ] total error: 3 / elpased: 921.356

i think about A in previous post, maybe nexttowardf is similar to x - numeric_limist<float>::epsilon() than x * (1.0f - numeric_limist<float>::epsilon()).
and, maybe addition's effect range is smaller than multiplication like B.

next, i think about B, maybe that works effectively than A. but that has many operation after the affecting than this way.
maybe, effect of smallize is become irrelevant by some operation.

so, epsilon can affects only too tiny range, it is easily erased.
and i tried to give effects at more behind.

i know this is not perfect. but i think this is more better way.
what are you think about this?

@2bbb

This comment has been minimized.

Show comment
Hide comment
@2bbb

2bbb May 23, 2015

Contributor

at first, we got pretty good result.
so, i compared stable version and new version that now we develop. (now 1 test only)
i gave same seed, and ran.

ver: now we developping ofRandom with seed = 0

[notice ] total error: 0 / elpased: 845.834

ver: stable ofRandom with seed = 0

[notice ] total error: 2933 / elpased: 848.226

i think this is great result. because accuracy becomes good, and almost cost does not increases.
so, i update for previous version with tiny optimize like this:

float ofRandom(float x, float y) {
    static constexpr float epsilon_smaller = 1.0f - FLT_EPSILON,
                           float_rand_max = static_cast<float>(RAND_MAX);
    // if there is no range, return the value
    if (fabsf(x - y) < FLT_EPSILON) return x;
    float high = MAX(x, y);
    float low = MIN(x, y);
    float range = high - low;
    float result = rand() / float_rand_max * range;
    result = result * epsilon_smaller + low;
    return result;
}

but i feel anxiety about result of new version error: 0.
oh, really...?
i wanna believe this is correct result.

i did push new version of ofRandom to this PR branch.
please check and test my code.

Contributor

2bbb commented May 23, 2015

at first, we got pretty good result.
so, i compared stable version and new version that now we develop. (now 1 test only)
i gave same seed, and ran.

ver: now we developping ofRandom with seed = 0

[notice ] total error: 0 / elpased: 845.834

ver: stable ofRandom with seed = 0

[notice ] total error: 2933 / elpased: 848.226

i think this is great result. because accuracy becomes good, and almost cost does not increases.
so, i update for previous version with tiny optimize like this:

float ofRandom(float x, float y) {
    static constexpr float epsilon_smaller = 1.0f - FLT_EPSILON,
                           float_rand_max = static_cast<float>(RAND_MAX);
    // if there is no range, return the value
    if (fabsf(x - y) < FLT_EPSILON) return x;
    float high = MAX(x, y);
    float low = MIN(x, y);
    float range = high - low;
    float result = rand() / float_rand_max * range;
    result = result * epsilon_smaller + low;
    return result;
}

but i feel anxiety about result of new version error: 0.
oh, really...?
i wanna believe this is correct result.

i did push new version of ofRandom to this PR branch.
please check and test my code.

@bilderbuchi

This comment has been minimized.

Show comment
Hide comment
@bilderbuchi

bilderbuchi May 23, 2015

Member

great analysis!

Member

bilderbuchi commented May 23, 2015

great analysis!

@2bbb

This comment has been minimized.

Show comment
Hide comment
@2bbb

2bbb May 23, 2015

Contributor

so, i found:

#include <iostream>

void printFloatAsHex(float f) { printf("%0x\n", *(reinterpret_cast<int *>(&f))); }
int main(int argc, char *argv[]) {
    printFloatAsHex((1.1f + 1.2f) + 1.3f);
    printFloatAsHex(1.1f + (1.2f + 1.3f));
}

results:

40666666
40666667

yeah, floating point is too difficult... 😃

Contributor

2bbb commented May 23, 2015

so, i found:

#include <iostream>

void printFloatAsHex(float f) { printf("%0x\n", *(reinterpret_cast<int *>(&f))); }
int main(int argc, char *argv[]) {
    printFloatAsHex((1.1f + 1.2f) + 1.3f);
    printFloatAsHex(1.1f + (1.2f + 1.3f));
}

results:

40666666
40666667

yeah, floating point is too difficult... 😃

@bilderbuchi

This comment has been minimized.

Show comment
Hide comment
@bilderbuchi

bilderbuchi May 23, 2015

Member

well, yes, that's floating point arithmetic for you... :-P If you wanna dive in, check out this.

Member

bilderbuchi commented May 23, 2015

well, yes, that's floating point arithmetic for you... :-P If you wanna dive in, check out this.

@2bbb

This comment has been minimized.

Show comment
Hide comment
@2bbb

2bbb May 23, 2015

Contributor

@bilderbuchi
yeah, i know that page in oracle.
many times, i tried to read, and slept soon everytime... 😪

Contributor

2bbb commented May 23, 2015

@bilderbuchi
yeah, i know that page in oracle.
many times, i tried to read, and slept soon everytime... 😪

@bilderbuchi

This comment has been minimized.

Show comment
Hide comment
@bilderbuchi

bilderbuchi May 23, 2015

Member

hehe I totally understand :D

Member

bilderbuchi commented May 23, 2015

hehe I totally understand :D

@2bbb

This comment has been minimized.

Show comment
Hide comment
@2bbb

2bbb May 23, 2015

Contributor

cool... 👏
i will try to read again!!

Contributor

2bbb commented May 23, 2015

cool... 👏
i will try to read again!!

@arturoc

This comment has been minimized.

Show comment
Hide comment
@arturoc

arturoc May 23, 2015

Member

@2bbb this works for me in every case including big ranges:

//--------------------------------------------------
float ofRandom(float max) {
    return max * rand() / float(RAND_MAX) * (1.0f-std::numeric_limits<float>::epsilon());
}

//--------------------------------------------------
float ofRandom(float x, float y) {
    // if there is no range, return the value
    if (x == y) return x;           // float == ?, wise? epsilon?
    float high = MAX(x,y);
    float low = MIN(x,y);
    return (low + ((high - low) * rand() / float(RAND_MAX))) * (1.0f-std::numeric_limits<float>::epsilon());
}

//--------------------------------------------------
float ofRandomf() {
    return 2.0 * rand() / float(RAND_MAX) * (1.0f-std::numeric_limits<float>::epsilon());
}

//--------------------------------------------------
float ofRandomuf() {
    return rand() / float(RAND_MAX) * (1.0f-std::numeric_limits<float>::epsilon());
}

we could refactor it a bit to reuse some bits of it but i think it's ok to leave it like this too being only a couple of functions

Member

arturoc commented May 23, 2015

@2bbb this works for me in every case including big ranges:

//--------------------------------------------------
float ofRandom(float max) {
    return max * rand() / float(RAND_MAX) * (1.0f-std::numeric_limits<float>::epsilon());
}

//--------------------------------------------------
float ofRandom(float x, float y) {
    // if there is no range, return the value
    if (x == y) return x;           // float == ?, wise? epsilon?
    float high = MAX(x,y);
    float low = MIN(x,y);
    return (low + ((high - low) * rand() / float(RAND_MAX))) * (1.0f-std::numeric_limits<float>::epsilon());
}

//--------------------------------------------------
float ofRandomf() {
    return 2.0 * rand() / float(RAND_MAX) * (1.0f-std::numeric_limits<float>::epsilon());
}

//--------------------------------------------------
float ofRandomuf() {
    return rand() / float(RAND_MAX) * (1.0f-std::numeric_limits<float>::epsilon());
}

we could refactor it a bit to reuse some bits of it but i think it's ok to leave it like this too being only a couple of functions

@bilderbuchi

This comment has been minimized.

Show comment
Hide comment
@bilderbuchi

bilderbuchi May 23, 2015

Member

+1 for refactoring a bit to reuse code, and +1 to an epsilon-based comparison in the if (x == y) return x bit.

Member

bilderbuchi commented May 23, 2015

+1 for refactoring a bit to reuse code, and +1 to an epsilon-based comparison in the if (x == y) return x bit.

@2bbb

This comment has been minimized.

Show comment
Hide comment
@2bbb

2bbb May 23, 2015

Contributor

i have three question.

first, is simple, i think, maybe ofRandomf is:

float ofRandomf() {
    return 2.0f * rand() / float(RAND_MAX) * (1.0f-std::numeric_limits<float>::epsilon()) - 1.0f;
}

second, about ofRandom(float max).
if we give negative value neg to ofRandom(neg) then maybe return value is in range (neg, 0].
isn't this incorrect?
i think correct range is [neg, 0)

third,
if rand() returns 0 then (low + ((high - low) * rand() / float(RAND_MAX))) * (1.0f-std::numeric_limits<float>::epsilon()); means low * (1.0f-std::numeric_limits<float>::epsilon());.
if low == 0.0f then okay.
but 0.0f < low then return value is less than low.
and low < 0.0f then return value is never be same as low. (is this okay? or not?)

{ // case 0.0f < low
    float low = 0.1f, high = 1.0f;
    float z = (low + ((high - low) * 0 / float(RAND_MAX))) * (1.0f-std::numeric_limits<float>::epsilon());
    if(z == low) printf("low == z\n");
    else if(z < low) printf("z < low\n");
    else printf("low < z\n");
    printf("z: %12.12f\n", z);
    printf("low: %12.12f\n", low);
    printf("\n");
}   
{ // case low < 0.0f
    float low = -0.1f, high = 1.0f;
    float z = (low + ((high - low) * 0 / float(RAND_MAX))) * (1.0f-std::numeric_limits<float>::epsilon());
    if(z == low) printf("low == z\n");
    else if(z < low) printf("z < low\n");
    else printf("low < z\n");
    printf("z: %12.12f\n", z);
    printf("low: %12.12f\n", low);
    printf("\n");
}

results:

z < low
z: 0.099999986589
low: 0.100000001490

low < z
z: -0.099999986589
low: -0.100000001490

so, i think correct code is like:

return low + (((high - low) * rand() / float(RAND_MAX))) * (1.0f-std::numeric_limits<float>::epsilon());
Contributor

2bbb commented May 23, 2015

i have three question.

first, is simple, i think, maybe ofRandomf is:

float ofRandomf() {
    return 2.0f * rand() / float(RAND_MAX) * (1.0f-std::numeric_limits<float>::epsilon()) - 1.0f;
}

second, about ofRandom(float max).
if we give negative value neg to ofRandom(neg) then maybe return value is in range (neg, 0].
isn't this incorrect?
i think correct range is [neg, 0)

third,
if rand() returns 0 then (low + ((high - low) * rand() / float(RAND_MAX))) * (1.0f-std::numeric_limits<float>::epsilon()); means low * (1.0f-std::numeric_limits<float>::epsilon());.
if low == 0.0f then okay.
but 0.0f < low then return value is less than low.
and low < 0.0f then return value is never be same as low. (is this okay? or not?)

{ // case 0.0f < low
    float low = 0.1f, high = 1.0f;
    float z = (low + ((high - low) * 0 / float(RAND_MAX))) * (1.0f-std::numeric_limits<float>::epsilon());
    if(z == low) printf("low == z\n");
    else if(z < low) printf("z < low\n");
    else printf("low < z\n");
    printf("z: %12.12f\n", z);
    printf("low: %12.12f\n", low);
    printf("\n");
}   
{ // case low < 0.0f
    float low = -0.1f, high = 1.0f;
    float z = (low + ((high - low) * 0 / float(RAND_MAX))) * (1.0f-std::numeric_limits<float>::epsilon());
    if(z == low) printf("low == z\n");
    else if(z < low) printf("z < low\n");
    else printf("low < z\n");
    printf("z: %12.12f\n", z);
    printf("low: %12.12f\n", low);
    printf("\n");
}

results:

z < low
z: 0.099999986589
low: 0.100000001490

low < z
z: -0.099999986589
low: -0.100000001490

so, i think correct code is like:

return low + (((high - low) * rand() / float(RAND_MAX))) * (1.0f-std::numeric_limits<float>::epsilon());
@2bbb

This comment has been minimized.

Show comment
Hide comment
@2bbb

2bbb May 23, 2015

Contributor

about second question in previous comment, i think answer is like this:

//--------------------------------------------------
float ofRandom(float max) {
    if(max == 0.0f) return 0.0f; // isn't this need?
    return MIN(0.0f, max) + ((fabs(max) * rand() / float(RAND_MAX))) * (1.0f-std::numeric_limits<float>::epsilon());
}
Contributor

2bbb commented May 23, 2015

about second question in previous comment, i think answer is like this:

//--------------------------------------------------
float ofRandom(float max) {
    if(max == 0.0f) return 0.0f; // isn't this need?
    return MIN(0.0f, max) + ((fabs(max) * rand() / float(RAND_MAX))) * (1.0f-std::numeric_limits<float>::epsilon());
}
@kylemcdonald

This comment has been minimized.

Show comment
Hide comment
@kylemcdonald

kylemcdonald May 23, 2015

Contributor

wow, this got twice as long when i wasn't looking!

@2bbb i think the correct range is [x, y), and it's unrelated to neg, 0 or pos values. because they you can ask for [x, y) or switch your arguments and you'll get [y, x) as you like. if there is a switch internally for neg, then you don't have that flexibility.

for your third question, it does seem like there is a problem there.

at this point, i'm very interested in using a library or other standard, since it seems like this is more complex topic than we initially realized, with all of us making mistakes -- but it's definitely fun to learn :)

Contributor

kylemcdonald commented May 23, 2015

wow, this got twice as long when i wasn't looking!

@2bbb i think the correct range is [x, y), and it's unrelated to neg, 0 or pos values. because they you can ask for [x, y) or switch your arguments and you'll get [y, x) as you like. if there is a switch internally for neg, then you don't have that flexibility.

for your third question, it does seem like there is a problem there.

at this point, i'm very interested in using a library or other standard, since it seems like this is more complex topic than we initially realized, with all of us making mistakes -- but it's definitely fun to learn :)

@2bbb

This comment has been minimized.

Show comment
Hide comment
@2bbb

2bbb May 23, 2015

Contributor

@kylemcdonald

maybe i can understand what you say about [x, y) and [y, x) with switch arguments. i feel "more flexible" too.
but why i thought like previous question is from two reasons.
one reason, i think [x, y) means x < y implicitly. (so, i feel this rule is majority in mathematics.)
and another one reason, but i can't understand why we do use the MAX and MIN in ofRandom(x, y).
so, i believe,

return low + (((high - low) * 0 / float(RAND_MAX))) * (1.0f-std::numeric_limits<float>::epsilon());

this expression returns correct range. and if we need a [x, y) as flexible, then this should be

return x + (((y - x) * 0 / float(RAND_MAX))) * (1.0f-std::numeric_limits<float>::epsilon());

i think. it isn't?

at last, i'm fun to learn too!!
and, i thought i wanna do fast aid about this problem. but it was too complex problem…
but, maybe we will complete it soon.
i wanna go next stage, in the world where we can use more good parts.
and but, but, but, at the same time, i feel chinese (and famous in japan too) proverb "温故知新" (means "learning from the past")

Contributor

2bbb commented May 23, 2015

@kylemcdonald

maybe i can understand what you say about [x, y) and [y, x) with switch arguments. i feel "more flexible" too.
but why i thought like previous question is from two reasons.
one reason, i think [x, y) means x < y implicitly. (so, i feel this rule is majority in mathematics.)
and another one reason, but i can't understand why we do use the MAX and MIN in ofRandom(x, y).
so, i believe,

return low + (((high - low) * 0 / float(RAND_MAX))) * (1.0f-std::numeric_limits<float>::epsilon());

this expression returns correct range. and if we need a [x, y) as flexible, then this should be

return x + (((y - x) * 0 / float(RAND_MAX))) * (1.0f-std::numeric_limits<float>::epsilon());

i think. it isn't?

at last, i'm fun to learn too!!
and, i thought i wanna do fast aid about this problem. but it was too complex problem…
but, maybe we will complete it soon.
i wanna go next stage, in the world where we can use more good parts.
and but, but, but, at the same time, i feel chinese (and famous in japan too) proverb "温故知新" (means "learning from the past")

@bilderbuchi

This comment has been minimized.

Show comment
Hide comment
@bilderbuchi

bilderbuchi May 23, 2015

Member

question: can we already use C++11 features, or is there still a supported platform lacking it? @arturoc ? then we could just go with uniform_real_distribution?

Member

bilderbuchi commented May 23, 2015

question: can we already use C++11 features, or is there still a supported platform lacking it? @arturoc ? then we could just go with uniform_real_distribution?

@ofTheo

This comment has been minimized.

Show comment
Hide comment
@ofTheo

ofTheo May 23, 2015

Contributor

my only comment on this amazing amazing discussion is that we need to triple check that we don't change the current behavior.

i.e.: if ofRandom(0, 1) currently gives out 0.999999999 as the max value but the new function ofRandom(0, 1) gives out 1.0 as the max value, that could cause all sorts of bugs elsewhere.

learning a lot from this thread!! :)

Contributor

ofTheo commented May 23, 2015

my only comment on this amazing amazing discussion is that we need to triple check that we don't change the current behavior.

i.e.: if ofRandom(0, 1) currently gives out 0.999999999 as the max value but the new function ofRandom(0, 1) gives out 1.0 as the max value, that could cause all sorts of bugs elsewhere.

learning a lot from this thread!! :)

@bilderbuchi

This comment has been minimized.

Show comment
Hide comment
@bilderbuchi

bilderbuchi May 23, 2015

Member

new function ofRandom(0, 1) gives out 1.0 as the max value

I'd consider this a bug.

Member

bilderbuchi commented May 23, 2015

new function ofRandom(0, 1) gives out 1.0 as the max value

I'd consider this a bug.

@arturoc

This comment has been minimized.

Show comment
Hide comment
@arturoc

arturoc May 24, 2015

Member

we can't use c++11 completely yet. we can use somethings like auto, but not thread, mutex... i guess anything that is keywords is valid everything else except if it's on tr1 is not. the platform holding us back is osx 32bit

@ofTheo which of the new functions? :)

Member

arturoc commented May 24, 2015

we can't use c++11 completely yet. we can use somethings like auto, but not thread, mutex... i guess anything that is keywords is valid everything else except if it's on tr1 is not. the platform holding us back is osx 32bit

@ofTheo which of the new functions? :)

@bilderbuchi

This comment has been minimized.

Show comment
Hide comment
@bilderbuchi

bilderbuchi May 24, 2015

Member

hm, it seems that uniform_real was part of TR1, too, so maybe we could use that, and maybe special-case the name for osx 32bit - the API is the same.

Member

bilderbuchi commented May 24, 2015

hm, it seems that uniform_real was part of TR1, too, so maybe we could use that, and maybe special-case the name for osx 32bit - the API is the same.

@2bbb

This comment has been minimized.

Show comment
Hide comment
@2bbb

2bbb May 24, 2015

Contributor

i began to thinking about like "do really we need line of if(x == y)?"
we can get same results if we don't check.
this line effects only case x == y.
and now we don't need divide by x - y, i.e. don't get error if we don't check
moreover i think x==y is rare.

so, i think so that there is a value to be compared about cost.

  • checking x == y on every call
  • do same processing without the check.

which one is better?

@ofTheo

cause of this PR is about problem you say.
i.e. we need ofRandom(0.0f, 1.0f) doesn't gives out 1.0f, but now stable version of ofRandom(0.0f, 1.0f) will gives out 1.0f on some environments.
i think so too, check is very important. because ofRandom is kind of most fundamental function.
we must so test about boundary value and cost, i think.

Contributor

2bbb commented May 24, 2015

i began to thinking about like "do really we need line of if(x == y)?"
we can get same results if we don't check.
this line effects only case x == y.
and now we don't need divide by x - y, i.e. don't get error if we don't check
moreover i think x==y is rare.

so, i think so that there is a value to be compared about cost.

  • checking x == y on every call
  • do same processing without the check.

which one is better?

@ofTheo

cause of this PR is about problem you say.
i.e. we need ofRandom(0.0f, 1.0f) doesn't gives out 1.0f, but now stable version of ofRandom(0.0f, 1.0f) will gives out 1.0f on some environments.
i think so too, check is very important. because ofRandom is kind of most fundamental function.
we must so test about boundary value and cost, i think.

@kylemcdonald

This comment has been minimized.

Show comment
Hide comment
@kylemcdonald

kylemcdonald May 25, 2015

Contributor

@ofTheo, @2bbb just said this above but i wanted to repeat it: this PR exists because ofRandom*() is broken right now. they all give the maximum value sometimes, it's just rare (around 1 in 50 million calls). so this discussion is about how to fix that bug, without introducing new bugs, and without slowing down ofRandom significantly.

@arturoc one solution would be to implement this with c++11 but have a #define for 32-bit code that is a slower implementation, and eventually remove it.

Contributor

kylemcdonald commented May 25, 2015

@ofTheo, @2bbb just said this above but i wanted to repeat it: this PR exists because ofRandom*() is broken right now. they all give the maximum value sometimes, it's just rare (around 1 in 50 million calls). so this discussion is about how to fix that bug, without introducing new bugs, and without slowing down ofRandom significantly.

@arturoc one solution would be to implement this with c++11 but have a #define for 32-bit code that is a slower implementation, and eventually remove it.

@2bbb

This comment has been minimized.

Show comment
Hide comment
@2bbb

2bbb May 25, 2015

Contributor

@kylemcdonald
okay, i understood. and sorry.
i should have been posted separately those are other opinions not related to this problem.
(i will do reconsideration about i had too excited.)

I concentrate about this issue: [x, y).

thanks!

Contributor

2bbb commented May 25, 2015

@kylemcdonald
okay, i understood. and sorry.
i should have been posted separately those are other opinions not related to this problem.
(i will do reconsideration about i had too excited.)

I concentrate about this issue: [x, y).

thanks!

@kylemcdonald

This comment has been minimized.

Show comment
Hide comment
@kylemcdonald

kylemcdonald May 27, 2015

Contributor

@2bbb i think your idea about [-x, 0) vs (-x, 0] is important! but i think it's not the meaning of ofRandom()

what i wrote to @ofTheo above was just to clarify that this PR is solving a bug. he wrote "ofRandom(0, 1) currently gives out 0.999999999 as the max value" which you have shown to be incorrect, and i wanted to make sure this was clear.

Contributor

kylemcdonald commented May 27, 2015

@2bbb i think your idea about [-x, 0) vs (-x, 0] is important! but i think it's not the meaning of ofRandom()

what i wrote to @ofTheo above was just to clarify that this PR is solving a bug. he wrote "ofRandom(0, 1) currently gives out 0.999999999 as the max value" which you have shown to be incorrect, and i wanted to make sure this was clear.

@ofTheo

This comment has been minimized.

Show comment
Hide comment
@ofTheo

ofTheo May 27, 2015

Contributor

@kylemcdonald I was just giving an example of a change that could be problematic :)
"i.e.: if ofRandom(0, 1) currently gives out 0.999999999 as the max value ...."

as its currently not behaving that way, totally agree that in this case changing its behavior is a bugfix :)

Contributor

ofTheo commented May 27, 2015

@kylemcdonald I was just giving an example of a change that could be problematic :)
"i.e.: if ofRandom(0, 1) currently gives out 0.999999999 as the max value ...."

as its currently not behaving that way, totally agree that in this case changing its behavior is a bugfix :)

@kylemcdonald

This comment has been minimized.

Show comment
Hide comment
@kylemcdonald

kylemcdonald May 28, 2015

Contributor

oh, whoops! it looks like i accidentally a word.

Contributor

kylemcdonald commented May 28, 2015

oh, whoops! it looks like i accidentally a word.

@2bbb

This comment has been minimized.

Show comment
Hide comment
@2bbb

2bbb May 30, 2015

Contributor

ofRandom(2500.0f, 2600.0f) breaks this bugfix easily.
range = high - low is narrow, and low is large.
so, i think, effect of * (1.0f - epsilon) is canceled by large low.

nmmmm... 😢

uniform_real_distribution...?

Contributor

2bbb commented May 30, 2015

ofRandom(2500.0f, 2600.0f) breaks this bugfix easily.
range = high - low is narrow, and low is large.
so, i think, effect of * (1.0f - epsilon) is canceled by large low.

nmmmm... 😢

uniform_real_distribution...?

@jedahan

This comment has been minimized.

Show comment
Hide comment
@jedahan

jedahan May 30, 2015

Contributor

I tried nexttoward, and it seemed pretty reliable (on latest OSX) with this code, which I have not yet tested in a modified oF.

Its pretty late here, so theres probably some obvious bugs with the tests or I didn't run them long enough...

#include <stdlib.h>
#include <iostream>
#include <sys/time.h>
#include <unistd.h>
#include <cmath>

#define MAX(x,y) (((x) > (y)) ? (x) : (y))
#define MIN(x,y) (((x) < (y)) ? (x) : (y))

float ofRandom(float x, float y) {
  float high = MAX(x, y);
  float low = MIN(x, y);
  float range = high - low;
  range -= nexttoward(range, 0.0l);
  float denominator = RAND_MAX / range;
  float result = rand() / denominator;
  result += low;
  return result;
}

float ofRandom(float max) {
  return ofRandom(0.0f, max);
}

float ofRandomf() {
  return ofRandom(-1.0f, 1.0f);
}

float ofRandomuf() {
  return ofRandom(1.0f);
}

void ofRandomufTest() {
  for(int n = 0; true; n++) {
    for(int i = 0; i < 100000000; i++) {
      float x = ofRandomuf();
      if(x > 0.99999999) {
        printf("booo, ofRandomuf() returned %12.12f after %d calls\n", x, n);
      }
    }
    printf("%d runs of 100000000 tries\n", n);
  }
}

void ofRandomIncreasingRangeTest() {
  for(float from = 1; from < std::numeric_limits<float>::max(); from*=2){
    for(int i=0; i < 100000000; i++) {
      float x = ofRandom(-from, from);
      if( x < -from || x >= from ) {
        printf("dang, %12.12f is out of range of [%12.12f, %12.12f)\n", x, -from, from);
      }
    }
    printf("passed %f\n", from);
  }
}

void ofSeedRandom() {
  struct timeval tv;
  gettimeofday(&tv, 0);
  long int n = (tv.tv_sec ^ tv.tv_usec) ^ getpid();
  srand(n);
}

int main() {
  ofSeedRandom();
//ofRandomufTest();
  ofRandomIncreasingRangeTest();
}
Contributor

jedahan commented May 30, 2015

I tried nexttoward, and it seemed pretty reliable (on latest OSX) with this code, which I have not yet tested in a modified oF.

Its pretty late here, so theres probably some obvious bugs with the tests or I didn't run them long enough...

#include <stdlib.h>
#include <iostream>
#include <sys/time.h>
#include <unistd.h>
#include <cmath>

#define MAX(x,y) (((x) > (y)) ? (x) : (y))
#define MIN(x,y) (((x) < (y)) ? (x) : (y))

float ofRandom(float x, float y) {
  float high = MAX(x, y);
  float low = MIN(x, y);
  float range = high - low;
  range -= nexttoward(range, 0.0l);
  float denominator = RAND_MAX / range;
  float result = rand() / denominator;
  result += low;
  return result;
}

float ofRandom(float max) {
  return ofRandom(0.0f, max);
}

float ofRandomf() {
  return ofRandom(-1.0f, 1.0f);
}

float ofRandomuf() {
  return ofRandom(1.0f);
}

void ofRandomufTest() {
  for(int n = 0; true; n++) {
    for(int i = 0; i < 100000000; i++) {
      float x = ofRandomuf();
      if(x > 0.99999999) {
        printf("booo, ofRandomuf() returned %12.12f after %d calls\n", x, n);
      }
    }
    printf("%d runs of 100000000 tries\n", n);
  }
}

void ofRandomIncreasingRangeTest() {
  for(float from = 1; from < std::numeric_limits<float>::max(); from*=2){
    for(int i=0; i < 100000000; i++) {
      float x = ofRandom(-from, from);
      if( x < -from || x >= from ) {
        printf("dang, %12.12f is out of range of [%12.12f, %12.12f)\n", x, -from, from);
      }
    }
    printf("passed %f\n", from);
  }
}

void ofSeedRandom() {
  struct timeval tv;
  gettimeofday(&tv, 0);
  long int n = (tv.tv_sec ^ tv.tv_usec) ^ getpid();
  srand(n);
}

int main() {
  ofSeedRandom();
//ofRandomufTest();
  ofRandomIncreasingRangeTest();
}
@jedahan

This comment has been minimized.

Show comment
Hide comment
@jedahan

jedahan May 30, 2015

Contributor

More correct for ofRandomIncreasingRangeTest would be to do from++ instead of from*=2, I was just getting impatient and wanted to test big ranges...

Contributor

jedahan commented May 30, 2015

More correct for ofRandomIncreasingRangeTest would be to do from++ instead of from*=2, I was just getting impatient and wanted to test big ranges...

@2bbb

This comment has been minimized.

Show comment
Hide comment
@2bbb

2bbb May 31, 2015

Contributor

hi, @jedahan
i think, maybe, this ofRandom(x, y) returns x in most cases.
because, range -= nexttoward(range, 0.0l); is approximately 0.0f. therefore, float denominator = RAND_MAX / range be too large number, and result be approximately 0.0f.

so, i came up with a idea but this is little ugly.
i make epsilon larger reasonably. like (fabs(low) + 1.0f) * epsilon().
but, i think this one pass case "range is narrow, but low is large".

i will test with some situation.

float ofRandom(float x, float y) {
    // if there is no range, return the value
    if (x == y) return x;           // float == ?, wise? epsilon?
    float high = MAX(x,y);
    float low = MIN(x,y);
    return low + (((high - low) * rand() / float(RAND_MAX))) * (1.0f - (fabs(low) + 1.0f) * std::numeric_limits<float>::epsilon());
}
Contributor

2bbb commented May 31, 2015

hi, @jedahan
i think, maybe, this ofRandom(x, y) returns x in most cases.
because, range -= nexttoward(range, 0.0l); is approximately 0.0f. therefore, float denominator = RAND_MAX / range be too large number, and result be approximately 0.0f.

so, i came up with a idea but this is little ugly.
i make epsilon larger reasonably. like (fabs(low) + 1.0f) * epsilon().
but, i think this one pass case "range is narrow, but low is large".

i will test with some situation.

float ofRandom(float x, float y) {
    // if there is no range, return the value
    if (x == y) return x;           // float == ?, wise? epsilon?
    float high = MAX(x,y);
    float low = MIN(x,y);
    return low + (((high - low) * rand() / float(RAND_MAX))) * (1.0f - (fabs(low) + 1.0f) * std::numeric_limits<float>::epsilon());
}
@2bbb

This comment has been minimized.

Show comment
Hide comment
@2bbb

2bbb May 31, 2015

Contributor

oh yeah, @jedahan
maybe i noticed?
did you wanted to do this?

  float range = high - low;
  range = nexttoward(range, 0.0l);
Contributor

2bbb commented May 31, 2015

oh yeah, @jedahan
maybe i noticed?
did you wanted to do this?

  float range = high - low;
  range = nexttoward(range, 0.0l);
@arturoc

This comment has been minimized.

Show comment
Hide comment
@arturoc

arturoc May 31, 2015

Member

@2bbb the solution i posted a while ago is working for me with 2500..2600 without problem:

//--------------------------------------------------
float ofRandom(float max) {
    return max * rand() / float(RAND_MAX) * (1.0f-std::numeric_limits<float>::epsilon());
}

//--------------------------------------------------
float ofRandom(float x, float y) {
    // if there is no range, return the value
    if (x == y) return x;           // float == ?, wise? epsilon?
    float high = MAX(x,y);
    float low = MIN(x,y);
    return (low + ((high - low) * rand() / float(RAND_MAX))) * (1.0f-std::numeric_limits<float>::epsilon());
}

//--------------------------------------------------
float ofRandomf() {
    return 2.0 * rand() / float(RAND_MAX) * (1.0f-std::numeric_limits<float>::epsilon());
}

//--------------------------------------------------
float ofRandomuf() {
    return rand() / float(RAND_MAX) * (1.0f-std::numeric_limits<float>::epsilon());
}

we should definetly be using c++11 solution so if someone wants to add this + ifdef to use uniform_real_distribution for everything but osx? not sure if you can detect 64bit osx in an ifdef

we should somehow solve osx32 not being compatible with c++11 asap, deprecating it or porting it too to libc++. otherwise we are going to begin to fill the core with ifdefs since there's a lot of really useful classes in c++11's std that we can't use otherwise

Member

arturoc commented May 31, 2015

@2bbb the solution i posted a while ago is working for me with 2500..2600 without problem:

//--------------------------------------------------
float ofRandom(float max) {
    return max * rand() / float(RAND_MAX) * (1.0f-std::numeric_limits<float>::epsilon());
}

//--------------------------------------------------
float ofRandom(float x, float y) {
    // if there is no range, return the value
    if (x == y) return x;           // float == ?, wise? epsilon?
    float high = MAX(x,y);
    float low = MIN(x,y);
    return (low + ((high - low) * rand() / float(RAND_MAX))) * (1.0f-std::numeric_limits<float>::epsilon());
}

//--------------------------------------------------
float ofRandomf() {
    return 2.0 * rand() / float(RAND_MAX) * (1.0f-std::numeric_limits<float>::epsilon());
}

//--------------------------------------------------
float ofRandomuf() {
    return rand() / float(RAND_MAX) * (1.0f-std::numeric_limits<float>::epsilon());
}

we should definetly be using c++11 solution so if someone wants to add this + ifdef to use uniform_real_distribution for everything but osx? not sure if you can detect 64bit osx in an ifdef

we should somehow solve osx32 not being compatible with c++11 asap, deprecating it or porting it too to libc++. otherwise we are going to begin to fill the core with ifdefs since there's a lot of really useful classes in c++11's std that we can't use otherwise

@2bbb

This comment has been minimized.

Show comment
Hide comment
@2bbb

2bbb Jun 1, 2015

Contributor

@arturoc

nmmmmmm...

sorry.
was my English wrong? or was my text unclearly?
i wanted to point out bug in your code with this post

okay, i know your code

// original arturoc version
float ofRandom(float x, float y) {
    // if there is no range, return the value
    if (x == y) return x;           // float == ?, wise? epsilon?
    float high = MAX(x,y);
    float low = MIN(x,y);
    return (low + ((high - low) * rand() / float(RAND_MAX))) * (1.0f-std::numeric_limits<float>::epsilon());
}

is always ofRandom(2500, 2600) < 2600.
but, when rand() == 0.

    (low + ((high - low) * rand() / float(RAND_MAX))) * (1.0f-std::numeric_limits<float>::epsilon())
==> (low + ((high - low) * 0 / float(RAND_MAX))) * (1.0f-std::numeric_limits<float>::epsilon())
==> (low + (0 / float(RAND_MAX))) * (1.0f-std::numeric_limits<float>::epsilon())
==> (low + 0) * (1.0f-std::numeric_limits<float>::epsilon())
==> low * (1.0f-std::numeric_limits<float>::epsilon())

i.e. ofRandom(2500, 2600) < 2500.
i thought this is bug. and i fixed your code like:

// 2bit fixed
float ofRandom(float x, float y) {
    // if there is no range, return the value
    if (x == y) return x;           // float == ?, wise? epsilon?
    float high = MAX(x,y);
    float low = MIN(x,y);
    return low + ((high - low) * rand() / float(RAND_MAX)) * (1.0f-std::numeric_limits<float>::epsilon());
}

so, i moved low to outside of parentheses.
but, but, this version returns just 2600.0f when rand() == RAND_MAX.

for this reason, i posted this.


and, i agree to using uniform_real_distribution with ifdef if we can.

Contributor

2bbb commented Jun 1, 2015

@arturoc

nmmmmmm...

sorry.
was my English wrong? or was my text unclearly?
i wanted to point out bug in your code with this post

okay, i know your code

// original arturoc version
float ofRandom(float x, float y) {
    // if there is no range, return the value
    if (x == y) return x;           // float == ?, wise? epsilon?
    float high = MAX(x,y);
    float low = MIN(x,y);
    return (low + ((high - low) * rand() / float(RAND_MAX))) * (1.0f-std::numeric_limits<float>::epsilon());
}

is always ofRandom(2500, 2600) < 2600.
but, when rand() == 0.

    (low + ((high - low) * rand() / float(RAND_MAX))) * (1.0f-std::numeric_limits<float>::epsilon())
==> (low + ((high - low) * 0 / float(RAND_MAX))) * (1.0f-std::numeric_limits<float>::epsilon())
==> (low + (0 / float(RAND_MAX))) * (1.0f-std::numeric_limits<float>::epsilon())
==> (low + 0) * (1.0f-std::numeric_limits<float>::epsilon())
==> low * (1.0f-std::numeric_limits<float>::epsilon())

i.e. ofRandom(2500, 2600) < 2500.
i thought this is bug. and i fixed your code like:

// 2bit fixed
float ofRandom(float x, float y) {
    // if there is no range, return the value
    if (x == y) return x;           // float == ?, wise? epsilon?
    float high = MAX(x,y);
    float low = MIN(x,y);
    return low + ((high - low) * rand() / float(RAND_MAX)) * (1.0f-std::numeric_limits<float>::epsilon());
}

so, i moved low to outside of parentheses.
but, but, this version returns just 2600.0f when rand() == RAND_MAX.

for this reason, i posted this.


and, i agree to using uniform_real_distribution with ifdef if we can.

@arturoc

This comment has been minimized.

Show comment
Hide comment
@arturoc

arturoc Jun 1, 2015

Member

oh, i see yeah sorry you are right

Member

arturoc commented Jun 1, 2015

oh, i see yeah sorry you are right

@arturoc

This comment has been minimized.

Show comment
Hide comment
@arturoc

arturoc Jun 1, 2015

Member

what about:

//--------------------------------------------------
float ofRandom(float x, float y) {
    float high = max(x,y);
    float low = min(x,y);
    return max(low, (low + ((high - low) * rand() / float(RAND_MAX))) * (1.0f-std::numeric_limits<float>::epsilon()));
}

that works for me without problem in both the low and high range also it doesn't need the if == since the calculation is correct even when low == high

Member

arturoc commented Jun 1, 2015

what about:

//--------------------------------------------------
float ofRandom(float x, float y) {
    float high = max(x,y);
    float low = min(x,y);
    return max(low, (low + ((high - low) * rand() / float(RAND_MAX))) * (1.0f-std::numeric_limits<float>::epsilon()));
}

that works for me without problem in both the low and high range also it doesn't need the if == since the calculation is correct even when low == high

@arturoc

This comment has been minimized.

Show comment
Hide comment
@arturoc

arturoc Jun 1, 2015

Member

this also works:

//--------------------------------------------------
float ofRandom(float x, float y) {
    if(x == y) return x;
    float high = max(x,y);
    float low = min(x,y);
    return low + (((high * (1.0f-std::numeric_limits<float>::epsilon()) - low) * rand() / float(RAND_MAX)));
}

and it's probably easier for the compiler to optimize that first condition than the max at the end in the other version with the result of a calculation. at least if the passed values are literals. not sure what happens if the passed values are results of another calculation, then the if(x==y) could give unexpected results so the previous version is probably more correct.

Member

arturoc commented Jun 1, 2015

this also works:

//--------------------------------------------------
float ofRandom(float x, float y) {
    if(x == y) return x;
    float high = max(x,y);
    float low = min(x,y);
    return low + (((high * (1.0f-std::numeric_limits<float>::epsilon()) - low) * rand() / float(RAND_MAX)));
}

and it's probably easier for the compiler to optimize that first condition than the max at the end in the other version with the result of a calculation. at least if the passed values are literals. not sure what happens if the passed values are results of another calculation, then the if(x==y) could give unexpected results so the previous version is probably more correct.

@2bbb

This comment has been minimized.

Show comment
Hide comment
@2bbb

2bbb Jun 2, 2015

Contributor

i ran this code:

#include <iostream>
#include <cmath>
#include <string>
#include <cstdlib>
#include <limits>

using namespace std;

struct FloatValueTest {
    virtual float testFunc(float low, float high, unsigned long r) {
        return 0.0f;
    }

    bool test_stuff(float low, float high) {
        if(high == testFunc(low, high, RAND_MAX)) {
            cout << "error: testFunc(" << low << ", " << high << ", RAND_MAX)" << " == " << high << endl;
            return false;
        } else if(high < testFunc(low, high, RAND_MAX)) {
            cout << "error: testFunc(" << low << ", " << high << ", RAND_MAX)" << " > " << high << endl;
            return false;
        }

        if(testFunc(low, high, 0) < low) {
            cout << "error: testFunc(" << low << ", " << high << ", 0)" << " < " << low << endl;
            return false;
        }

        return true;
    }

    bool test() {
        for(int i = 1; i <= 10000; i++) if(!test_stuff(-1.0f / i, 1.0f / i)) return false;
        for(int i = 1; i <= 10000; i++) if(!test_stuff(i * -100.0f, i * 100.0f)) return false;
        for(int i = 1; i <= 10000; i++) if(!test_stuff(i * 100.0f, i * 100.0f + 1.0f)) return false;
        for(int i = 1; i <= 10000; i++) if(!test_stuff(0.0f, i * 500.0f + 1.0f)) return false;
        return true;
    }
};

struct Test1 : FloatValueTest {
    // #issuecomment-107341477
    virtual float testFunc(float low, float high, unsigned long r) {
        return max(low, (low + ((high - low) * r / float(RAND_MAX))) * (1.0f-std::numeric_limits<float>::epsilon()));
    }
};

struct Test2 : FloatValueTest {
    // #issuecomment-107347547
    virtual float testFunc(float low, float high, unsigned long r) {
        return low + (high * (1.0f - std::numeric_limits<float>::epsilon()) - low) * (r / float(RAND_MAX));
    }
};

struct Test3 : FloatValueTest{
    // #issuecomment-107103350
    virtual float testFunc(float low, float high, unsigned long r) {
        return low + (((high - low) * (r / float(RAND_MAX)))) * (1.0f - (fabs(low) + 1.0f) * std::numeric_limits<float>::epsilon());
    }
};

int main(int argc, char *argv[]) {
    cout << "Test1" << endl;
    if(Test1().test()) {
        cout << "pass all test!" << endl;
    }
    cout << endl;

    cout << "Test2" << endl;
    if(Test2().test()) {
        cout << "pass all test!" << endl;
    }
    cout << endl;

    cout << "Test3" << endl;
    if(Test3().test()) {
        cout << "pass all test!" << endl;
    }
    cout << endl;

    return EXIT_SUCCESS;
}

and results is:

Test1
pass all test!

Test2
error: testFunc(-0.0909091, 0.0909091, RAND_MAX) == 0.0909091

Test3
pass all test!

Test1
Test2
Test3

i am writing in haste.

Contributor

2bbb commented Jun 2, 2015

i ran this code:

#include <iostream>
#include <cmath>
#include <string>
#include <cstdlib>
#include <limits>

using namespace std;

struct FloatValueTest {
    virtual float testFunc(float low, float high, unsigned long r) {
        return 0.0f;
    }

    bool test_stuff(float low, float high) {
        if(high == testFunc(low, high, RAND_MAX)) {
            cout << "error: testFunc(" << low << ", " << high << ", RAND_MAX)" << " == " << high << endl;
            return false;
        } else if(high < testFunc(low, high, RAND_MAX)) {
            cout << "error: testFunc(" << low << ", " << high << ", RAND_MAX)" << " > " << high << endl;
            return false;
        }

        if(testFunc(low, high, 0) < low) {
            cout << "error: testFunc(" << low << ", " << high << ", 0)" << " < " << low << endl;
            return false;
        }

        return true;
    }

    bool test() {
        for(int i = 1; i <= 10000; i++) if(!test_stuff(-1.0f / i, 1.0f / i)) return false;
        for(int i = 1; i <= 10000; i++) if(!test_stuff(i * -100.0f, i * 100.0f)) return false;
        for(int i = 1; i <= 10000; i++) if(!test_stuff(i * 100.0f, i * 100.0f + 1.0f)) return false;
        for(int i = 1; i <= 10000; i++) if(!test_stuff(0.0f, i * 500.0f + 1.0f)) return false;
        return true;
    }
};

struct Test1 : FloatValueTest {
    // #issuecomment-107341477
    virtual float testFunc(float low, float high, unsigned long r) {
        return max(low, (low + ((high - low) * r / float(RAND_MAX))) * (1.0f-std::numeric_limits<float>::epsilon()));
    }
};

struct Test2 : FloatValueTest {
    // #issuecomment-107347547
    virtual float testFunc(float low, float high, unsigned long r) {
        return low + (high * (1.0f - std::numeric_limits<float>::epsilon()) - low) * (r / float(RAND_MAX));
    }
};

struct Test3 : FloatValueTest{
    // #issuecomment-107103350
    virtual float testFunc(float low, float high, unsigned long r) {
        return low + (((high - low) * (r / float(RAND_MAX)))) * (1.0f - (fabs(low) + 1.0f) * std::numeric_limits<float>::epsilon());
    }
};

int main(int argc, char *argv[]) {
    cout << "Test1" << endl;
    if(Test1().test()) {
        cout << "pass all test!" << endl;
    }
    cout << endl;

    cout << "Test2" << endl;
    if(Test2().test()) {
        cout << "pass all test!" << endl;
    }
    cout << endl;

    cout << "Test3" << endl;
    if(Test3().test()) {
        cout << "pass all test!" << endl;
    }
    cout << endl;

    return EXIT_SUCCESS;
}

and results is:

Test1
pass all test!

Test2
error: testFunc(-0.0909091, 0.0909091, RAND_MAX) == 0.0909091

Test3
pass all test!

Test1
Test2
Test3

i am writing in haste.

@2bbb

This comment has been minimized.

Show comment
Hide comment
@2bbb

2bbb Jun 3, 2015

Contributor

i check more test, and results is this code is most correct.
(this code returns not correct value when fabs(low) is too large... omg)

and i pushed with latest codes.

Contributor

2bbb commented Jun 3, 2015

i check more test, and results is this code is most correct.
(this code returns not correct value when fabs(low) is too large... omg)

and i pushed with latest codes.

@kylemcdonald

This comment has been minimized.

Show comment
Hide comment
@kylemcdonald

kylemcdonald Jun 3, 2015

Contributor

going to merge this now and leave the c++11 and uniform_real_distribution discussion for another issue :) thanks @2bbb for all the testing!

Contributor

kylemcdonald commented Jun 3, 2015

going to merge this now and leave the c++11 and uniform_real_distribution discussion for another issue :) thanks @2bbb for all the testing!

kylemcdonald added a commit that referenced this pull request Jun 3, 2015

Merge pull request #3842 from 2bbb/fix_ofRandom_returns_just_maximum_…
…value

change denominator of ofRandomuf from float to double.

@kylemcdonald kylemcdonald merged commit dfb7d4c into openframeworks:master Jun 3, 2015

@ofTheo

This comment has been minimized.

Show comment
Hide comment
@ofTheo

ofTheo Jun 3, 2015

Contributor
Contributor

ofTheo commented Jun 3, 2015

@2bbb

This comment has been minimized.

Show comment
Hide comment
@2bbb

2bbb Jun 3, 2015

Contributor

very very thanks everyone. and sorry for my poor english.
i study english and C++, (and IEEE754) again, and wanna discuss with you on other hardcore issue again!
see you soon!

Contributor

2bbb commented Jun 3, 2015

very very thanks everyone. and sorry for my poor english.
i study english and C++, (and IEEE754) again, and wanna discuss with you on other hardcore issue again!
see you soon!

@2bbb 2bbb deleted the 2bbb:fix_ofRandom_returns_just_maximum_value branch Jun 3, 2015

@ofTheo ofTheo referenced this pull request Aug 13, 2015

Open

gaussian noise #224

@kylemcdonald kylemcdonald referenced this pull request Nov 1, 2015

Closed

OF 0.9.0 release checklist #4104

6 of 6 tasks complete
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment