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
1 change: 1 addition & 0 deletions src/Magnum/Math/CMakeLists.txt
Original file line number Diff line number Diff line change
Expand Up @@ -49,6 +49,7 @@ set(MagnumMath_HEADERS
Quaternion.h
Packing.h
PackingBatch.h
Random.h
Range.h
RectangularMatrix.h
StrictWeakOrdering.h
Expand Down
131 changes: 131 additions & 0 deletions src/Magnum/Math/Random.h
Original file line number Diff line number Diff line change
@@ -0,0 +1,131 @@
#ifndef Magnum_Math_Random_h
#define Magnum_Math_Random_h

// TO DO Licence things.

#include <random>
#include <chrono>
#include "Magnum/Types.h"
#include "Magnum/Math/Constants.h"
#include "Magnum/Math/Vector2.h"
#include "Magnum/Math/Vector3.h"
#include "Magnum/Math/Quaternion.h"
#include "Magnum/Math/Functions.h"

namespace Magnum
{
namespace Math
{

namespace Implementation
{
static std::seed_seq seeds{{
static_cast<std::uintmax_t>(std::random_device{}()),
static_cast<std::uintmax_t>(std::chrono::steady_clock::now()
.time_since_epoch()
.count()),
}};

class RandomGenerator
{
public:
RandomGenerator() = delete;

template <typename T>
static typename std::enable_if<std::is_same<Int, T>::value, T>::type
generate(T start = -Magnum::Math::Constants<T>::inf(),
T end = Magnum::Math::Constants<T>::inf())
{
return std::uniform_int_distribution<T>{start, end}(generator());
}

template <typename T>
static typename std::enable_if<std::is_same<Float, T>::value, T>::type
generate(T start = -Magnum::Math::Constants<T>::inf(),
T end = Magnum::Math::Constants<T>::inf())
{
return std::uniform_real_distribution<T>{start, end}(generator());
}

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;

return g;
}
};
} // namespace Implementation
namespace Random
{
template <class T = Float>
T randomUnsignedScalar() // range [0, 1]
{
return Implementation::RandomGenerator::generate<T>(static_cast<T>(0.0f),
static_cast<T>(1.0f));
}
template <class T = Float>
T randomSignedScalar() // range [-1, 1]
{
return Implementation::RandomGenerator::generate(static_cast<T>(-1.0f), static_cast<T>(1.0f));
}

template <class T = Float>
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 :)

}

template <class T = Float>
Vector3<T> randomUnitVector3()
{
auto a = Implementation::RandomGenerator::generate(0.0f, 2 * Math::Constants<T>::pi());
auto z = randomSignedScalar();
auto r = sqrt<T>(1 - z * z);
return {r * std::cos(a), r * std::sin(a), z};
}
template <class T = Float>
Vector2<T> randomPointInACircle() // always length < 1
{
while (true)
{
auto p = Vector2<T>(
randomSignedScalar(), randomSignedScalar());
if (p.length() >= 1)
continue;
return p;
}
}

template <class T = Float>
Vector3<T> randomPointInASphere() // always length < 1
{

while (true)
{
auto p = Vector3<T>(randomSignedScalar(),
randomSignedScalar(),
randomSignedScalar());
if (p.length() >= 1)
continue;
return p;
}
}
sariug marked this conversation as resolved.
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?])


template <class T = Float>
Quaternion<T> randomRotation()
{
return Quaternion<T>({randomSignedScalar<T>(), randomSignedScalar<T>(), randomSignedScalar<T>()}, randomSignedScalar<T>()).normalized();
}

} // namespace Random

} // namespace Math
} // namespace Magnum

#endif
6 changes: 6 additions & 0 deletions src/Magnum/Math/Test/CMakeLists.txt
Original file line number Diff line number Diff line change
Expand Up @@ -70,6 +70,8 @@ corrade_add_test(MathStrictWeakOrderingTest StrictWeakOrderingTest.cpp LIBRARIES

corrade_add_test(MathMatrixBenchmark MatrixBenchmark.cpp LIBRARIES MagnumMathTestLib)

corrade_add_test(MathRandomTest RandomTest.cpp LIBRARIES MagnumMathTestLib)

set_property(TARGET
MathVectorTest
MathMatrixTest
Expand All @@ -85,6 +87,8 @@ set_property(TARGET

MathDistanceTest
MathIntersectionTest

MathRandomTest
APPEND PROPERTY COMPILE_DEFINITIONS "CORRADE_GRACEFUL_ASSERT")

set_target_properties(
Expand Down Expand Up @@ -130,4 +134,6 @@ set_target_properties(

MathConfigurationValueTest
MathStrictWeakOrderingTest

MathRandomTest
PROPERTIES FOLDER "Magnum/Math/Test")
112 changes: 112 additions & 0 deletions src/Magnum/Math/Test/RandomTest.cpp
Original file line number Diff line number Diff line change
@@ -0,0 +1,112 @@
#include <Corrade/TestSuite/Tester.h>
#include <Corrade/Utility/DebugStl.h>

#include "Magnum/Math/Random.h"

namespace Magnum
{
namespace Math
{

namespace Test
{
namespace
{

struct RandomTest : Corrade::TestSuite::Tester
{
explicit RandomTest();

void unsignedScalar();
void signedScalar();
void unitVector2();
void unitVector3();
void pointInACircle();
void pointInASphere();
void randomRotation();
};

typedef Vector<2, Float> Vector2;
typedef Vector<3, Float> Vector3;
typedef Math::Constants<Float> Constants;

RandomTest::RandomTest()
{
addTests({&RandomTest::unsignedScalar,
&RandomTest::signedScalar,
&RandomTest::unitVector2,
&RandomTest::unitVector3,
&RandomTest::pointInACircle,
&RandomTest::pointInASphere,
&RandomTest::randomRotation});
}

// void RandomTest::construct() {
// constexpr Matrix4x4 a = {Vector4(3.0f, 5.0f, 8.0f, -3.0f),
// Vector4(4.5f, 4.0f, 7.0f, 2.0f),
// Vector4(1.0f, 2.0f, 3.0f, -1.0f),
// Vector4(7.9f, -1.0f, 8.0f, -1.5f)};
// CORRADE_COMPARE(a, Matrix4x4(Vector4(3.0f, 5.0f, 8.0f, -3.0f),
// Vector4(4.5f, 4.0f, 7.0f, 2.0f),
// Vector4(1.0f, 2.0f, 3.0f, -1.0f),
// Vector4(7.9f, -1.0f, 8.0f, -1.5f)));

// CORRADE_VERIFY((std::is_nothrow_constructible<Matrix4x4, Vector4, Vector4, Vector4, Vector4>::value));
// }
sariug marked this conversation as resolved.
Show resolved Hide resolved
void RandomTest::unsignedScalar()
{
for (std::size_t i = 0; i < 10; i++)
sariug marked this conversation as resolved.
Show resolved Hide resolved
{
CORRADE_VERIFY(Math::Random::randomUnsignedScalar<Float>() < 1.000000001);
CORRADE_VERIFY(Math::Random::randomUnsignedScalar<Float>() > -.000000001);
sariug marked this conversation as resolved.
Show resolved Hide resolved
}
}

void RandomTest::signedScalar()
{
for (std::size_t i = 0; i < 10; i++)
{
CORRADE_VERIFY(Math::Random::randomUnsignedScalar<Float>() < 1.000000001);
CORRADE_VERIFY(Math::Random::randomUnsignedScalar<Float>() > -1.000000001);
}
}

void RandomTest::unitVector2()
{
for (std::size_t i = 0; i < 10; i++)
{
CORRADE_COMPARE((Math::Random::randomUnitVector2()).length(),1.0f);
}
}
void RandomTest::unitVector3()
{
for (std::size_t i = 0; i < 10; i++)
{
CORRADE_COMPARE((Math::Random::randomUnitVector3()).length(),1.0f);
}
}

void RandomTest::pointInACircle(){
for (std::size_t i = 0; i < 10; i++)
{
CORRADE_VERIFY((Math::Random::randomPointInACircle()).length()<1.0f);
}
}
void RandomTest::pointInASphere(){
for (std::size_t i = 0; i < 10; i++)
{
CORRADE_VERIFY((Math::Random::randomPointInASphere()).length()<1.0f);
}
}
void RandomTest::randomRotation(){
for (std::size_t i = 0; i < 10; i++)
{
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% ).

} // namespace
} // namespace Test
} // namespace Math
} // namespace Magnum

CORRADE_TEST_MAIN(Magnum::Math::Test::RandomTest)