kyonifer / koma

A scientific computing library for Kotlin. https://kyonifer.github.io/koma
Other
270 stars 23 forks source link

Created PCG random number generator #82

Closed peastman closed 5 years ago

peastman commented 5 years ago

Implements #80.

When creating the default random number generator, I've currently hardcoded the seed value as 0. It probably would be better to make it nondeterministic so you'll get different random numbers every time you run your program. But I can't figure out how to do that without writing multiplatform code, which really seems excessive! Kotlin 1.3 will add functions like getTimeNanos(), plus the whole kotlin.random package. Until that's released, is there some place to grab a single number worth of entropy from without using platform specific code?

peastman commented 5 years ago
koma-core-api/common/src/koma/internal/randn.kt:63:9: error: using 'synchronized(Any, () -> R): R' is an error. Do not use 'synchronized' function in Kotlin/Native code
        synchronized(this) {
        ^

Apparently synchronized() isn't supported in Kotlin/Native. In fact, it seems they're pushing a totally different threading model in which memory is never directly shared between threads. How do you suggest handling this?

peastman commented 5 years ago

I'd say let's wait for 1.3, and open some issues to track these regressions in the meantime.

Sounds good.

Thus I'd recommend for now we just replace the synchronized blocks with NOPs on native and leave it unsafe on that platform.

That makes sense. It seems consistent with the recommended approach to threading on native. I can create a function syncNotNative() to calls synchronized() on JVM and JS, and is a NOP on native.

peastman commented 5 years ago

I've revised the distribution tests to each run 20 times with reproducible seed values.

peastman commented 5 years ago

I'm not sure what's going on with the CI failure. If I'm reading it correctly, it's saying that everything passes on Windows and Mac, but one test fails on Linux. Any idea what could be causing that? The test in question passes on my computer.

kyonifer commented 5 years ago

I'm not sure what's going on with the CI failure. If I'm reading it correctly, it's saying that everything passes on Windows and Mac, but one test fails on Linux. Any idea what could be causing that? The test in question passes on my computer.

Yep thats what's happening. I'm able to reproduce the failure locally on Ubuntu 18.04. Looks like the issue is that you aren't resetting the spare output of Box-Muller when setting the seed. This causes it to use the spare value for the first Gaussian requested after the seed was reset instead of generating a new one. However it only does that if a spare is available, so a 50% chance. This causes it to yield a different sequence with the same seed and ultimately the test to complain on ubuntu when run with gradle caching enabled. This patch fixes it for me:

--- a/koma-core-api/common/src/koma/internal/randn.kt
+++ b/koma-core-api/common/src/koma/internal/randn.kt
@@ -84,6 +84,7 @@ class KomaRandom {
      */
     fun setSeed(seed: Long) {
         syncNotNative(this) {
+            gaussianIsValid = false
             state1 = 0
             state2 = 0
             nextLongUnsafe()

Should hopefully fix the CI as well.

peastman commented 5 years ago

And the tests must get run in a different order on some platforms than others, so on some platforms it's produced an even number of values before then and on others it has produced an odd number. It's fixed now.

kyonifer commented 5 years ago

Yeah. And if you turn off the daemon, or run it again without running :clean first, or mess with the persistent caching it didn't happen. So the ordering is extremely brittle. Luckily this one reproduced on a clean first run on Linux.

This PR looks good here. I'll open those follow-up issues we mentioned and bring it in.