fix: Use Fisher-Yates for shuffledIndices - #221
Open
SamuelSchlesinger wants to merge 3 commits into
Open
Conversation
shuffledIndices seeds its loop with a random bound, randomR (1, k - 1), so indices above that bound are never touched and roughly half the vector keeps its original order. The inner step then draws from randomR (1, maxInd) rather than (0, maxInd), which is Sattolo's algorithm and never leaves the head element in place. Over 400 seeds of a ten-element shuffle, positions 0 and 1 never keep their index and positions 4 through 9 keep it far too often. Shuffling a single index passes a reversed range to randomR, whose result is not specified; here it swaps past the end of a one-element vector.
The loop started from a random bound instead of the last index, leaving everything above it in place, and each step drew from randomR (1, maxInd) so the head element could never stay put. Walk from the last index down to 1, swapping with a draw from randomR (0, i). The loop is empty for k <= 1, which also removes the out-of-bounds swap on a one-row frame.
Contributor
|
keen eye. can you do a test for randomness please? https://cnut1648.github.io/files/posts/Test_for_rand.pdf |
Author
|
Done. Replaced the fixed-point check with two χ² tests at α = 0.001, following the frequency test in that note (Knuth 3.3.2):
Seeds are fixed so both are deterministic. Fisher-Yates scores 114.7 and 83.4 against bounds of 172.4 and 126.1; the previous implementation scores 137088 and 114988 on the same procedure. |
Replace the fixed-point rate check with two chi-squared tests at alpha = 0.001: one over the full permutation distribution (n = 5, 12000 draws, df 119), which also catches correlated positions, and Knuth's frequency test over the position-by-item table (n = 10, 5000 draws, df 81). Seeds are fixed, so both are deterministic. Fisher-Yates scores 114.7 and 83.4 against bounds of 172.4 and 126.1; the previous shuffle scores 137088 and 114988 on the same procedure.
SamuelSchlesinger
force-pushed
the
fix/shuffle-fisher-yates
branch
from
August 24, 2026 12:36
c6fe9f9 to
e2259a8
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
The shuffle from #172 isn't Fisher-Yates: it picks a random number of steps up front and never lets an element stay in place, so the permutation is biased, and on a one-row frame
randomR (1, 0)leads to an out-of-bounds swap (index out of bounds (1,1)for about half of seeds). Replace it with the standard backwards Fisher-Yates.Seeded outputs change as a result. Tests: the single-row case, and a fixed-point-rate check across seeds.