jobjo / popper

Property-based testing at ease
ISC License
43 stars 1 forks source link

(Sample.Float.range 0. 1.) can return a negative number #72

Open anentropic opened 1 year ago

anentropic commented 1 year ago

I made a sampler like:

let sample_ggv3 low high =
  let* x = Sample.Float.range low high in
  let* y = Sample.Float.range low high in
  let* z = Sample.Float.range low high in
  Sample.return (Gg.V3.v x y z)

which I sample from in a test like:

    let* points =
      Sample.with_log
        "Sample points"
        (fun fmt points' -> pp_point_list fmt points')
        Sample.List.(non_empty @@ sample_ggv3 0. 1.)
    in

My test is failing and log output shows:

Sample points = [(0 0 0); (-4.65661e-10 0 0)]

So it looks like Sample.Float.range 0. 1. has returned -4.65661e-10 as a sample value, which is < 0

I am guessing there is maybe a floating point precision issue in the float_range function so that it only approximately respects the min and max? https://github.com/jobjo/popper/blob/main/src/lib/sample.ml#L112

I was unable to reproduce the failure by testing just float sampling:

let test_float_sample =
  test @@ fun () ->
    let* n = Sample.Float.range 0. 1. in
    all [
      greater_equal_than Comparator.float n 0.;
      less_equal_than Comparator.float n 1.;
    ]

...or at least that test case reports "Passed 300 samples"

anentropic commented 1 year ago

Is there a pattern I could use akin to https://hypothesis.readthedocs.io/en/latest/data.html#filtering ?

I came up with this workaround to use in my test:

let strict_float_range low high =
  let rec sample () =
    let* x = Sample.Float.range low high in
    if x < low || x > high then
      sample ()
    else
      Sample.return x
  in
  sample ()

Seems to work... Just wondering if there could be any subtle issue with discarding samples in the sampler I need to be aware of?

jobjo commented 1 year ago

Hi, @anentropic , thanks for the report. Will have a look in a bit.

So it looks like Sample.Float.range 0. 1. has returned -4.65661e-10 as a sample value, which is < 0

Seems like a bug indeed. Will check.

Is there a pattern I could use akin to https://hypothesis.readthedocs.io/en/latest/data.html#filtering ?

Not at the moment. We could certainly add a filter combinator although with too narrow filters it may be slow. People may not realize the semantics (that is the generator has to discard a a lot of samples). Feel free to push a PR if you are interested in contributing!