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

Shuffle isn't correct #14

Open
niyaznigmatullin opened this issue Oct 20, 2017 · 3 comments
Open

Shuffle isn't correct #14

niyaznigmatullin opened this issue Oct 20, 2017 · 3 comments

Comments

@niyaznigmatullin
Copy link

fun <T:Comparable<T>>shuffle(items:MutableList<T>):List<T>{
    val rg : Random = Random()
    for (i in 0..items.size - 1) {
        val randomPosition = rg.nextInt(items.size)
        val tmp : T = items[i]
        items[i] = items[randomPosition]
        items[randomPosition] = tmp
    }
    return items
}

This algorithm doesn't shuffle it correctly, not every permutation is equiprobable. You have to do:

val randomPosition = rg.nextInt(i + 1)

to fix it.

More information:
https://blog.codinghorror.com/the-danger-of-naivete/

@peheje
Copy link

peheje commented Nov 11, 2017

Maybe this is correct

import java.util.*

val random = SplittableRandom()

fun wrongShuffle(x: MutableList<Int>): List<Int> {
    for (i in 0 until x.size) {
        val n = random.nextInt(x.size)
        x[i] = x[n].also { x[n] = x[i] }
    }
    return x
}

fun maybeCorrectShuffle(x: MutableList<Int>): List<Int> {
    for (i in x.size downTo 2) {
        // downTo 2 because if down to 1: 1-1 = 0 so we would swap idx 0 with idx nextInt(0) which would fail.
        val n = random.nextInt(i)
        x[i-1] = x[n].also { x[n] = x[i-1] }
    }
    return x
}

fun main(args: Array<String>) {
    val iterations = 1_000_000
    val cards = listOf(1, 2, 3)
    val permutations = mutableListOf<List<Int>>()
    val counts = mutableMapOf<List<Int>, Int>()

    for (i in 0 until iterations) {
        val shuffledCards = maybeCorrectShuffle(cards.toMutableList())
        // Collections.shuffle(shuffledCards)
        permutations.add(shuffledCards)
    }

    println(permutations)

    for (perm in permutations) {
        if (counts.containsKey(perm)) counts[perm] = counts[perm]!! + 1 else counts[perm] = 1
    }

    println(counts)
}

@niyaznigmatullin
Copy link
Author

Actually, you would swap 0 with nextInt(1) which is also 0, so there is no need, for sure.

@peheje
Copy link

peheje commented Nov 12, 2017

Ah true. But yea you can skip going down to index 0 as the swap would always be with itself. Thanks for the interesting read at codinghorr niyaznigmatullin.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants