-
Notifications
You must be signed in to change notification settings - Fork 0
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
Reorganisation of the repo structure #4
base: master
Are you sure you want to change the base?
Conversation
Kerl13
commented
May 6, 2020
- Reference C implementations of the different RNGs are moved inside their respective OCaml modules;
- OCaml implementations are tested against the corresponding C reference.
What is the status of this? Can I help? Review, maybe? |
Yep, review and tell if you think of a better organisation |
Where would you put the TestU01 bindings? I don't know so much what to think about the organisation, because this repository features very different things, namely:
So I guess I would expect these three aspects of this repository being separated in three directories at the root of this repo? Those are simply ideas; and opinion on this? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I put a few comments, but their are just very random things and actually the PR can be merged whenever we want, from my point of view.
Oh, also, something that actually hasn't changed in this PR is the benchmarking tests. This is probably a job for an other PR, but shouldn't we seed the Random generators (on similar seeds) before testing? Like go from:
ocaml-rng-experiments/prng/src/bench.ml
Lines 28 to 29 in e29fb07
B.Test.create ~name:"Random.float" (fun () -> | |
Random.float 1.); |
to something like:
B.Test.create_with_initialization ~name:"Random.float" (fun `init ->
Random.init 4; (* please don't *)
fun () -> Random.float 1.);
(untested; from memory; might not be exactly this)
Probably yes! But I take the opportunity to postpone this to another PR ;) |
OK, I moved this to #10! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That all looks good to me! I sent quite a few comments; most of them are just here because I felt like trolling.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes!
Two conversations that I think can just be marked as resolved and then I have nothing else to say! |