Skip to content
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

Math/Random and its tests implementation #432

Open
wants to merge 11 commits into
base: master
Choose a base branch
from
Open

Conversation

sariug
Copy link

@sariug sariug commented Apr 6, 2020

Hello hello,

Did some simple implementation; would be happy to develop it further with the suggestions.
Prepared for one engine ("mt19937")

@sariug sariug changed the title Math/Random amnd its tests implementation Math/Random and its tests implementation Apr 6, 2020
@mosra mosra added this to the 2020.0a milestone Apr 8, 2020
Copy link
Owner

@mosra mosra left a comment

Choose a reason for hiding this comment

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

I really appreciate that you did a test as well 👍

Since this is your first contribution, I'm not commenting on coding style -- let's get this working first :)

src/Magnum/Math/Test/RandomTest.cpp Outdated Show resolved Hide resolved
public:
static std::mt19937 &generator()
{
static std::mt19937 g{seeds};
Copy link
Owner

Choose a reason for hiding this comment

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

static is problematic because of thread safety issues, global constructors/destructors etc. What do you think about this?

Math::RandomGenerator g; // the user is required to instantiate this first

auto a = Math::randomSignedScalar(g);
auto b = Math::randomUnitVector2(g);

The RandomGenerator thus wouldn't be in Implementation anymore, but will be exposed to the user, even though its only use would be to get passed to the random*() functions.

Copy link
Author

Choose a reason for hiding this comment

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

I thought about it, and tried to keep minimalistic(however you are right for the thread safety), to keep it as a 1-step function.

if you have no other proposal and think that this would be the best, then why not :)

Copy link
Owner

Choose a reason for hiding this comment

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

Not sure if best, but I didn't come up with anything better yet.

I think this should be also robust in case I decide to implement my own random generator later.

Copy link
Author

Choose a reason for hiding this comment

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

Implemented some stuff. Wondering how you would find these. I feel quiteee doubtful.(Especially would love to hide the "generate" function;

Vector2<T> randomUnitVector2()
{
auto a = Implementation::RandomGenerator::generate(0.0f, 2 * Math::Constants<T>::pi());
return {std::cos(a), std::sin(a)};
Copy link
Owner

Choose a reason for hiding this comment

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

I guess this is where things become hard 😅

I actually have no idea here -- will the distribution be still uniform if sin/cos is used? I guess it will? Ideally this would be without the extra overhead of trig functions, but I don't have any idea if that's doable ... in this thread on SO they use a Gaussian distribution as an input, but .. ¯\_(ツ)_/¯

If you have better references than me, mention them here please :)

Copy link
Author

@sariug sariug Apr 8, 2020

Choose a reason for hiding this comment

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

For this case sin and cos shall not be get affected ? (sin (+ + - - )[50%], cos (+ - - + )[50%] )
So this shall be as accurate as the generator itself ?

Can you also comment about what I read here ? It looks accurate according to this. The main discussion seems like "getting 3 random numbers and normalizing" vs "2 numbers(theta and height) and calculating". The latter seems more accurate, which is sin/cos.

Copy link
Owner

Choose a reason for hiding this comment

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

Yes that's a great reference, thanks -- a link to it should go in the documentation. I didn't absorb it fully yet, but yeah it sounds like a good proof.

getting 3 random numbers and normalizing

this is what you do for quaternions tho .. can the two-/three-dimensional case be extended for those as well?

Copy link
Author

Choose a reason for hiding this comment

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

Quaternion is link here.

Added both to the code :)

src/Magnum/Math/Random.h Outdated Show resolved Hide resolved
bool randomBool()
{
return static_cast<bool>(randomUnsignedScalar<Int>());
}
Copy link
Owner

Choose a reason for hiding this comment

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

Hmm, do we actually need this? :) The randomUnsignedScalar() for integers isn't really useful either. Maybe instead add a static_assert(IsFloatingPoint<T>::value, ""); to the RandomGenerator and remove everything related to integers? Those can be always added back later. (The trait is in Magnum/Math/TypeTraits.h.)

Copy link
Author

Choose a reason for hiding this comment

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

Well, like i said in the beginning, a initiation for a new magnum functionality :) Can grow later with demand :)

Deleting/Fixing. :)

Copy link
Author

Choose a reason for hiding this comment

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

I first deleted, and then put it back haha :D
From our initial discussion:

Float randomUnsignedScalar(); // range [0, 1]
Float randomSignedScalar(); // range [-1, 1]
Vector2 randomPointOnACircle(); // always length = 1
Vector3 randomPointOnASphere(); // also
Quaternion randomRotation(); // always normalized

Maybe keeping it with integers to not to cast anything if we roll a dice?(randomUnsignedScalar<int>(1,6))or to see if that girl is single?(randomUnsignedScalar<int>(0,1)[bool to cast, yes. maybe type traits for numerics?])

{
CORRADE_COMPARE(Math::Random::randomRotation().length(), 1.0f);
}
}
Copy link
Owner

Choose a reason for hiding this comment

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

It would be great to have some sort of distribution verification in the tests, to ensure we're not accidentally skewing the distribution to something non-uniform -- how good are your statistics skills? :)

Found this on SO, it suggests using a Chi Squared test. I never did such a thing myself tho :D

Copy link
Author

Choose a reason for hiding this comment

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

We have easter incoming ! :)

Copy link
Author

Choose a reason for hiding this comment

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

Well, I put a chi square test. In testing, I couldnt see a "tolerant" testing scheme(Chi square let 1 fail; like 99% ).

@mosra
Copy link
Owner

mosra commented Apr 16, 2020

Okay, great job! Everything seems to compile and pass tests except a few cases where

Value error_count is not less than 2, actual is 2 but expected < 2

I'm not sure how bad this is tho, seems to be depending on how good the standard implementation is? :)

I'll need some time to write the docs etc., but I think this can go in 🎉

@sariug
Copy link
Author

sariug commented Apr 16, 2020

Great news !

Value error_count is not less than 2, actual is 2 but expected < 2

I'm not sure how bad this is tho, seems to be depending on how good the standard implementation is? :)

So this happens in chisquare I guess, which the randomness in belong to std engine..
Let me know if i can support anyhow !

@mosra
Copy link
Owner

mosra commented Apr 17, 2020

TODOs for me (adding a comment so I don't forget):

@sariug
Copy link
Author

sariug commented Aug 23, 2020

Hey hey. Currently I was checking out Perlin noise(I've seen your "SmootherStep" :D ). Would you also like that to be implemented as an extension of this ?

Some noise here:
image

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Development

Successfully merging this pull request may close these issues.

2 participants