Math/Random and its tests implementation #432
Conversation
sariug
changed the title
src/Magnum/Math/Random.h
Outdated
| public: | ||
| static std::mt19937 &generator() | ||
| { | ||
| static std::mt19937 g{seeds}; |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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 :)
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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)}; |
There was a problem hiding this comment.
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 :)
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
Quaternion is link here.
Added both to the code :)
src/Magnum/Math/Random.h
Outdated
| bool randomBool() | ||
| { | ||
| return static_cast<bool>(randomUnsignedScalar<Int>()); | ||
| } |
There was a problem hiding this comment.
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.)
There was a problem hiding this comment.
Well, like i said in the beginning, a initiation for a new magnum functionality :) Can grow later with demand :)
Deleting/Fixing. :)
There was a problem hiding this comment.
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); | ||
| } | ||
| } |
There was a problem hiding this comment.
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
There was a problem hiding this comment.
Well, I put a chi square test. In testing, I couldnt see a "tolerant" testing scheme(Chi square let 1 fail; like 99% ).
|
Okay, great job! Everything seems to compile and pass tests except a few cases where
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 🎉 |
|
Great news !
So this happens in chisquare I guess, which the randomness in belong to std engine.. |
|
TODOs for me (adding a comment so I don't forget):
|
|
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: |
Hello hello,
Did some simple implementation; would be happy to develop it further with the suggestions.
Prepared for one engine ("mt19937")