Use random-shuffle list shuffling instead of random >= 1.3#169
Use random-shuffle list shuffling instead of random >= 1.3#169mchav merged 6 commits intoDataHaskell:mainfrom
Conversation
|
Per the contributing guidelines, I tried to add a label (I assumed that was what "A tag (usually feat, documentation, refactor etc)" meant?) but it seems like I'm not allowed to do that 🤔 |
|
|
||
| shuffledIndices :: (RandomGen g) => g -> Int -> VU.Vector Int | ||
| shuffledIndices pureGen k = VU.fromList (fst (uniformShuffleList [0 .. (k - 1)] pureGen)) | ||
| shuffledIndices pureGen k = VU.fromList (shuffle' [0 .. (k - 1)] k pureGen) |
There was a problem hiding this comment.
Pretty unclear to me how to test this. I thought about a shuffle/un-shuffle identity test, but unshuffle didn't get me very far in search results 😅
Any thoughts?
There was a problem hiding this comment.
There are a couple of things I would test then.
-
test that the shuffling doesn't do anything else than shuffle. So basically sort the shuffled and unshuffled and see if it's equal to the same thing. This ensures that shuffling isn't doing anything else than permuting the indices.
-
check that shuffling with equivalent seeds result in the same shuffle.
That's about all I can think of
There was a problem hiding this comment.
Yeah. No need to round trip. Checking that shuffling preserves length (even when there are duplicates) is probably important. Plus that different seeds are different shuffle orders.
There was a problem hiding this comment.
Also on second thought the intermediate list allocation is wasteful. I'll add it as a GSOC task to implement fisher yates here.
There was a problem hiding this comment.
that task shouldn't be GSOC, it should be anyone! Also mwc-random does that I think.
There was a problem hiding this comment.
Why not? It seems simple and self contained enough since it's reading the algorithm and implementing it through.
There was a problem hiding this comment.
there could be some task pipeline for people who are not interested only in GSOC, but more interested generally in contributing!
|
I used this branch's package in the plot survey branch I started with no problems other than needing to roll my own list generator with |
|
@jisantuc i think there's an issue with the test module name. Once that's fixed this will be good to go. |
|
@mchav yeah I forgot to rename the module after I understood the test naming convention a little better. Fixed now though |
🤦🏻 @mchav I just noticed other commits on |
|
@mchav What's your timeline on the next Hackage release? |
|
This will prevent dataframe 0.5 from going into Stackage unless you will relax the random bounds... |
|
Nix will not have a problem if random 1.3 is allowed in Cabal, since the default version of random is that of in Stackage (v1.2 for LTS 24). I agree it would be better to relax bounds on random.
@jisantuc Is this just from Nix's diamond dependency problems from 1.3, or is this present in other package environments as well? |
|
@Ai-Ya-Ya it was just the diamond dependency problem and Also TIL "bounds regression" |
5 participants
Overview
This PR downgrades random from >= 1.3 to between 1.2 and 1.3. random 1.3+ plays poorly with some packages in the ecosystem.
Instead of
uniformShuffleListM, it usesshuffle'fromrandom-shuffle, which was already a test dependency.