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

This is a simple to use and efficient rng. #191

Closed
wants to merge 1 commit into from

Conversation

schets
Copy link
Contributor

@schets schets commented Jan 10, 2015

It provides an easy and reliable way of reproducible random numbers,
along with platform-independent serialization. There is also a thread-safe global rng, although it makes no promises about the order in which requests will be processed.

@andrekupka
Copy link
Member

Is there a specific reason to implement a new rng instead of using one from the c++ standard library. Further the Travis build fails.

@schets
Copy link
Contributor Author

schets commented Jan 10, 2015

Building in provides stronger guarantees of reproducibility, as well as guarantees that serialization is the same.

According to the standard, the stl rng implementations themselves should all have identical output. GCC and Clang both follow that atm, but I don't know about ICC or VC++. I wasn't able to find any information governing the serialization of generators. The standard ones all support stream output, but I don't know how well that translates across implementation.

Also, pretty much everything that's implemented in the class besides random, discard, and seed would need to be implemented anyways. The distributions that are provided by the stl to go from random uint64 -> something useful are not standardized and do actually differ across implementations, and I couldn't find anything about serialization portability.

@TheJJ TheJJ added lang: c++ Done in C++ code nice new thing ☺ A new feature that was not there before labels Jan 12, 2015
rng_tests.cpp
)

add_test_cpp(openage::rng::tests::rng_tests "test functionality of rng - this *may* fail due to a statistical anomaly, but is extremely unlikely")
Copy link
Member

Choose a reason for hiding this comment

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

lolwhat 😆
could you add some comment what could fail there?

@TheJJ
Copy link
Member

TheJJ commented Jan 12, 2015

Apart from the above comments, awesome work, code, docs and tests are pretty easy to read and understand. Nicely done.

@schets schets force-pushed the simple-rng branch 3 times, most recently from 79b6898 to 026a17c Compare January 12, 2015 17:21
It provides an easy and reliable way of reproducible random numbers,
along with platform-independent serialization.

std::ostream &rng::to_stream(std::ostream &out) const {
if((out << this->state[0] << " ").good() &&
!(out << this->state[1]).fail()) {
Copy link
Member

Choose a reason for hiding this comment

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

Why are you using !(...).fail() instead of good()?

@andrekupka
Copy link
Member

The code looks ok. Nevertheless it would be nice to have some comments about the mathematical background.

@TheJJ
Copy link
Member

TheJJ commented Jan 19, 2015

[vortigaunt]
agreeeed.
the sams must explain the algorithm in comments.
also the sams must rebase to resolve merge conflict.
[/vortigaunt]

@TheJJ
Copy link
Member

TheJJ commented Feb 12, 2015

i'm continuing the work in #221, thanks again.

@TheJJ TheJJ closed this Feb 12, 2015
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
lang: c++ Done in C++ code nice new thing ☺ A new feature that was not there before
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants