stedolan / crowbar

Property fuzzing for OCaml
MIT License
180 stars 31 forks source link

Add a shuffled list generator #50

Closed NathanReb closed 5 years ago

NathanReb commented 5 years ago

This PR adds a shuffle: 'a list -> 'a list gen generator using Fisher-Yates algorithm.

I tested it with a toy program and it seems to work pretty well!

Let me know what you think!

stedolan commented 5 years ago

Thanks! (I broke this by removing Const, so I pushed a fix to your branch before merging)

pqwy commented 5 years ago

Two points:

let rec sequence = function
  g::gs -> map [g; sequence gs] (fun x xs -> x::xs)
| [] -> const []

let shuffle arr =
  let n = Array.length arr in
  let xs = List.init n (fun i -> range ~min:i (n - i)) in
  map [sequence xs] @@ fun js ->
    let arr = Array.copy arr in
    js |> List.iteri (fun i j ->
      let t = arr.(i) in
      arr.(i) <- arr.(j); arr.(j) <- t);
    arr

(The bias induced by mod doesn't matter here.)

pqwy commented 5 years ago

Ping @stedolan who merged while I was writing, lest the comments gets lost in github's torrent of activity :) .

stedolan commented 5 years ago

I'm happier with the version that generates an immutable list, even if it involves some copying (I have some experiments that share substructures of generated testcases, so generating mutable arrays scares me a bit). I like your map version better than the dynamic_bind version, though.

pqwy commented 5 years ago

Do you want a PR, then?

NathanReb commented 5 years ago

I'm not sure I get the first point since the current version already uses an array internally to shuffle. Whether it is then copied to a list or an array is just a matter of preferences and I'm guessing people will use lists more often than arrays.

NathanReb commented 5 years ago

Ok you're actually suggesting to pass it an array instead of a list to save the first copy.

pqwy commented 5 years ago

The first point is minor. It's just that shuffling an array is clearly the primitive operation here, so why not expose it as such? Otherwise, shuffling arrays is array->list->array->shuffle->list->array, and shuffling, say, my third sequence type is seq->list->array->shuffle->list->seq.

The major point is dropping the monad.

stedolan commented 5 years ago

PR for the map version welcome! I do prefer the list version, though: I don't care much about the copy and I'd like to avoid mutable structures in the API.